-
Notifications
You must be signed in to change notification settings - Fork 2k
Delete label #2465
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.
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
Delete label #2465
Conversation
Welcome @shubhbapna! |
Thanks for the PR. I don't actually think that the tests in this test (either the tests that you added, or the tests that were in the file) actually test anything other than the fact that network calls are being made. Specifically it seems like we should be checking that the body of the PUT request looks correct (in all cases) and we don't do that. If you wanted to fix these tests, that would be awesome, but I don't necessarily want to gate this PR on fixing tests that were not something you added, so I'm ok to merge this PR and then file an issue to get these tests fixed in a different PR. Let me know what you think. |
cc @yue9944882 |
I went ahead and fixed the existing tests in #2471 @shubhbapna can you add similar tests to this PR? |
Yep will do! |
Should I wait for #2471 to be merged, pull those changes and then fix the tests? Or should I fix them right now and resolve any merge conflicts later? |
@shubhbapna overall LGTM, and #2471 has been merged, can you rebase onto the latest? |
@shubhbapna can you run the formatter by |
e1065ee
to
037e699
Compare
@yue9944882 @brendandburns Rebased, fixed the tests and ran the formatter |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, shubhbapna The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Resolves #2422
deleteLabel
method in theKubectlLabel
class