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.

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-apiserver test: Cleaning up clusters after the test is run #27407

Merged
merged 1 commit into from
Jun 15, 2016

Conversation

nikhiljindal
Copy link
Contributor

The test was not deleting clusters after it completes

cc @kubernetes/sig-cluster-federation @mml

@mml
Copy link
Contributor

mml commented Jun 15, 2016

Do we need both? It seems like the AfterEach stanza deletes everything anyway.

@nikhiljindal
Copy link
Contributor Author

Updated the comment to say that we test that delete works.
We also have AfterEach as a fallback in case test fails before that.

@mml mml added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2016
@nikhiljindal nikhiljindal added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jun 15, 2016
@ghost
Copy link

ghost commented Jun 15, 2016

nit: I would make the cluster deletion test a separate one (i.e. a separate "It" clause).

Also, I'm slightly confused. The cluster deletion in the AfterEach() clause has already been added in:

#26636

?

@nikhiljindal
Copy link
Contributor Author

The one in #26636 is for the clusters that federated-service added.
I havent verified it yet, but the hypothesis that @mml and I discussed is that if federated-service test is run after this test (i.e federation-apiserver test), then when it tries to create clusters it should get an error saying this cluster already exists (since federation-apiserver already created those clusters but did not delete them).
This PR is to prevent that error.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 15, 2016
@nikhiljindal nikhiljindal added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jun 15, 2016
@k8s-bot
Copy link

k8s-bot commented Jun 15, 2016

GCE e2e build/test passed for commit 912df9b.

@ghost
Copy link

ghost commented Jun 15, 2016

@nikhiljindal My apologies, yes, I was confusing two different sets of tests (federation-apiserver and federation-service). If we moved both of those into the same test file, we could remove all of the duplicate code. Besides that, the distinction is rather artificial in the context of e2e tests. e.g. both create services, it just happens that one person wrote one of them to test api server, and the other to test servicecontroller.

Lets put in a followup PR to merge these two sets of tests.

@nikhiljindal
Copy link
Contributor Author

Adding retest not required label since this PR only affects the federation e2e which is not tested by the merge bot anyway.

@lavalamp lavalamp merged commit 571c94a into kubernetes:master Jun 15, 2016
@lavalamp
Copy link
Member

Manual build cop merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants