Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clean up for qos.go #45018

Merged
merged 1 commit into from
May 9, 2017
Merged

Clean up for qos.go #45018

merged 1 commit into from
May 9, 2017

Conversation

ravisantoshgudimetla
Copy link
Contributor

What this PR does / why we need it:
Seems we are not using any of those functions.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #39148

Release note:

A small clean up to remove unnecessary functions.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 27, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Hi @ravisantoshgudimetla. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 27, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 27, 2017
@ravisantoshgudimetla
Copy link
Contributor Author

@k8s-bot test this please!

@k8s-ci-robot
Copy link
Contributor

@ravisantoshgudimetla: you can't request testing unless you are a kubernetes member.

In response to this:

@k8s-bot test this please!

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@jayunit100
Copy link
Member

jayunit100 commented Apr 27, 2017 •
edited
Loading

If this is needed, it needs a unit test.
If its not needed, makes sense to delete.
I'll run e2e's locally and make sure its not used somewhere .

@sjpotter
Copy link
Contributor

seems faily straight forward, as it just removed functions. If it passes e2e tests seems fine.

@jayunit100
Copy link
Member

@k8s-bot test this please.

@jayunit100
Copy link
Member

@k8s-bot ok to test.

@feiskyer
Copy link
Member

Looks reasonable. Waiting for CI green.

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 28, 2017
@jayunit100
Copy link
Member

I'm seeing unit test failures here........? @ravisantoshgudimetla if you see these to i guess this code is needed after all :).

--- FAIL: TestRefetchSchemaWhenValidationFails (0.09s)
        factory_test.go:311: expected 1 schema request, saw: 0
--- FAIL: TestValidateCachesSchema (0.07s)
        factory_test.go:360: expected 1 schema request, saw: 0
        factory_test.go:368: expected 1 schema request, saw: 0
        factory_test.go:379: expected 1 schema request, saw: 0
        factory_test.go:390: expected 1 schema request, saw: 0
FAIL
FAIL    k8s.io/kubernetes/pkg/kubectl/cmd/util  1.44

@ravisantoshgudimetla
Copy link
Contributor Author

So, I am kind of confused here for 2 reasons.

  • Why is another file's unit-test making a call to the function in another package altogether.
  • I just looked at those files and it seems they are making a APIServer call(a fake one) and no where related to code changes that I made.

I may be wrong in any of those assumptions and the tests could be simply flakes. Can we run the unit tests only again to see, if there are some failures?

In fact, the same test failed for my other PR as well:

https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/45018/pull-kubernetes-unit/28128/

@jayunit100
Copy link
Member

@k8s-bot unit test this

@ravisantoshgudimetla
Copy link
Contributor Author

This is flake again.

@jayunit100
Copy link
Member

@k8s-bot unit test this

@jayunit100
Copy link
Member

/assign @wojtek-t

@wojtek-t wojtek-t assigned yujuhong and unassigned wojtek-t May 2, 2017
@yujuhong yujuhong assigned derekwaynecarr and vishh and unassigned yujuhong May 2, 2017
@yujuhong
Copy link
Contributor

yujuhong commented May 2, 2017

@vishh @derekwaynecarr, is it ok to remove the functions or are they going to be used in the near future?

@jayunit100
Copy link
Member

jayunit100 commented May 3, 2017 •
edited
Loading

  • iirc general policy is that we don't have dead code in the codebase.
  • obviously makes sense to double check and defer to vish and derek here but probably shouldn't gate for too long...

IMO we should remove deadcode as a policy unless there is an obvious note in the source not to TODO: we will use this in the near future for XYZ. any other process wont scale to the 100s of PRs that are floating around.

@ravisantoshgudimetla
Copy link
Contributor Author

ping @vishh @derekwaynecarr

@vishh
Copy link
Contributor

vishh commented May 8, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 8, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ravisantoshgudimetla, vishh

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2017
@ravisantoshgudimetla
Copy link
Contributor Author

Seems to be an infra error. I cannot ask the bot to test it again. Can someone do it?

@vishh
Copy link
Contributor

vishh commented May 9, 2017 via email

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 45018, 45330)

@k8s-github-robot k8s-github-robot merged commit f036725 into kubernetes:master May 9, 2017
@ravisantoshgudimetla ravisantoshgudimetla deleted the cleanup_qos#39148 branch May 9, 2017 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@feiskyer feiskyer

@sjpotter sjpotter

@derekwaynecarr derekwaynecarr

@vishh vishh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dead code in qos.go?