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
e2e: allow unknown providers with a warning #70141
Conversation
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 thanks.
maybe @timothysc has historic knowledge of why we allowed unknown providers pre-refactor.
/assign @timothysc
x reference this PR which is on hold with the alternative solution: this tackles the problem in kubetest. if this current PR is merged we need to cancel the above PR.. /this-is-fine |
In response to this:
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. |
ca1645d
to
ae9ebd0
Compare
So your change exposed s bunch of brittleness in the tests, this is kind of a band-aide, but do we still need it? Are the other tests fixed? /hold |
@timothysc At the moment this band-aid is still needed. I prefer to have it in 1.13 with a deprecation announcement, then in 1.14 remove it (and enhance the error handling - |
@pohly please log an issue and add a // TODO comment to the top of the conversion referencing the issue(s), and I'll get this unblocked. |
Discussed at the kubeadm office hours, one upshot to this is that removing providers from core should be slightly easier since they just get promoted to no-op with a warning. We're also fixing tests to avoid this warning. |
The E2E refactoring tightened the sanity checking of the --provider parameter such that it only allowed known providers. That seemed to make sense because it catches typos, but it turned out that various callers depended on the "accept arbitrary provider value" behavior, therefore it gets restored.
ae9ebd0
to
bf08f5c
Compare
@timothysc I've file an issue and added a TODO in the code - I hope I understood correctly where you wanted that. |
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.
/approve
/hold cancel
@BenTheElder for LGTM b/c he knows the deets of how it's currently failing.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly, timothysc 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 |
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
thanks!
What type of PR is this?
/kind bug
What this PR does / why we need it:
#68483 unintentionally broke several use cases by tightening the validation of the
--provider
parameter such that thee2e.test
binary only accepts known providers.Whether that really is the right behavior needs more discussion (and in hindsight it probably isn't). In the meantime we need to restore the previous behavior quickly to get all CI jobs working again.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Refs #70058
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
/sig testing
/cc @BenTheElder
/cc @neolit123