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
kubeadm unit test initalize global variables #100201
Conversation
/assign @liggitt @neolit123 |
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.
i would have suggested a refector to not have these global variables (grumble), but the flags that use them are deprecated:
https://github.com/kubernetes/kubernetes/blob/92af6be262e69b3cad821fbd175504d27cd2466c/cmd/kubeadm/app/cmd/phases/init/certs.go#L68-L72
with the removal of these flags, TestCertsWithCSRs can be removed as well, because there is already coverage elsewhere for this feature.
/lgtm
/approve
/triage accepted |
Need milestone 1.21? |
/retest |
/retest Review the full test history for this PR. Silence the bot with an |
hmm failures seems related 👀 |
@@ -41,6 +41,12 @@ func (t *testCertsData) CertificateDir() string { return t.cfg.Certi | |||
func (t *testCertsData) CertificateWriteDir() string { return t.cfg.CertificatesDir } | |||
|
|||
func TestCertsWithCSRs(t *testing.T) { | |||
// restore global variables | |||
defer func() { |
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.
Jordan showed me the way :)
#100203 (comment)
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.
I mean… not using global vars anymore is better… but if the thing you're testing explicitly requires them, resetting once the test is over is good to do
we need milestone and lgtm @neolit123 @spiffxp |
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.
/lgtm
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.
/lgtm
/priority important-soon
/milestone v1.21
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, neolit123, spiffxp 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 |
/retest Review the full test history for this PR. Silence the bot with an |
1 similar comment
/retest Review the full test history for this PR. Silence the bot with an |
/kind failing-test
/kind flake
xref https://testgrid.k8s.io/sig-testing-canaries#ci-kubernetes-generate-make-test-count10
Before it fail to run more than one time, the global variables weren't restored and the next iteration was using the previous value in the code under test, however, the test itself was using the new value.
Initialising the variables at the beginning of the test make the test idempotent.