-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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
[Federation] Add an end-to-end test verifying that deleting a federated namespace deletes child replicasets. #41278
[Federation] Add an end-to-end test verifying that deleting a federated namespace deletes child replicasets. #41278
Conversation
Hi @perotinus. 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 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. |
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.
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. |
1 similar comment
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.
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. |
caf0f25
to
1bd1408
Compare
I signed it! |
A couple of minor nits but mostly looks good. Also, please rebase. Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed. test/e2e_federation/federated-namespace.go, line 103 at r1 (raw file):
By convention, test/e2e_federation/federated-namespace.go, line 143 at r1 (raw file):
I recommend pulling this and
Comments from Reviewable |
c7d231d
to
089c589
Compare
Thanks, @madhusudancs! PTAL when you get a chance. |
Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions. test/e2e_federation/federated-namespace.go, line 103 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
Done. test/e2e_federation/federated-namespace.go, line 143 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
Done. Comments from Reviewable |
Reviewed 2 of 3 files at r2. test/e2e_federation/federation-util.go, line 530 at r2 (raw file):
It is a convention to use // on each line for docstrings. Comments from Reviewable |
089c589
to
0eba31d
Compare
Review status: 2 of 3 files reviewed at latest revision, 2 unresolved discussions. test/e2e_federation/federation-util.go, line 530 at r2 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
Ah, OK. Done. Should we fix the other comments in this file, perhaps in a follow-up PR? Comments from Reviewable |
Reviewed 1 of 3 files at r2, 1 of 2 files at r3. test/e2e_federation/federation-util.go, line 530 at r2 (raw file): Previously, perotinus (Jonathan MacMillan) wrote…
Follow up PR for other instances would be great. I will LGTM and approve this PR. Comments from Reviewable |
/lgtm Reviewed 1 of 2 files at r3. Comments from Reviewable |
…ed namespace deletes child replicasets. Verifies kubernetes#38225. Also, remove a few custom package aliases.
0eba31d
to
2e7683d
Compare
[APPROVALNOTIFIER] This PR is APPROVED The following people have approved this PR: madhusudancs, perotinus Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion. test/e2e_federation/federation-util.go, line 530 at r2 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
Thanks! I'll start working on that now. Comments from Reviewable |
@k8s-bot verify test this |
@perotinus: you can't request testing unless you are a kubernetes member. In response to this comment:
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-bot ok to test |
@madhusudancs Can you re-LGTM? |
/lgtm |
Automatic merge from submit-queue (batch tested with PRs 41357, 41178, 41280, 41184, 41278) |
…namespace deletion. See kubernetes#41278 for the original test.
Automatic merge from submit-queue (batch tested with PRs 41505, 41484, 41544, 41514, 41022) [Federation] Fixes the newly-added test for replicaset deletion upon namespace deletion. See #41278 for the original test. **Release note**: ```release-note NONE ```
…namespace deletion. See kubernetes#41278 for the original test.
|
||
By(fmt.Sprintf("Verify that replicaset %s was deleted as well", rsName)) | ||
|
||
waitForReplicaSetToBeDeletedOrFail(f.FederationClientset, nsName, rsName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not require a wait here. Replicaset should have been deleted before the namespace was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean the delete namespace operation is synchronous, i.e. it won't return until the namespace cleanup is complete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the function that this test is using to delete namespace is synchronous. It waits for the namespace to be deleted before returning.
So here, we can directly check that the replicaset is gone. No need to wait again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After following this commentary, I'd suggest appending a suffix to deleteNamespace to indicate that it is synchronous (+Sync or +AndWait, whichever....).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use AndWait
in kubernetes tests often, so we can stick with that I guess.
Verifies #38225.
Also, remove a few custom package aliases.