-
Notifications
You must be signed in to change notification settings - Fork 191
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
pkg/payload/precondition: File shuffling, drop ClusterVersion argument, etc. #708
pkg/payload/precondition: File shuffling, drop ClusterVersion argument, etc. #708
Conversation
…gradeable_test To match our preferred spelling. The test file has had the outgoing spelling since it landed in bcd58d8 (loosen upgradeable condition to allow z-level upgrades, 2020-01-02, openshift#291).
The argument landed in b72e843 (Bug 1822844: Block z level upgrades if ClusterVersionOverridesSet set, 2020-04-30, openshift#364) for use by Upgradeable.Run. But even then, that method opens by retrieving a (possibly cached) ClusterVersion resource from the configured lister, so there's no need to pass the explicit argument. We should save explicit inputs for things that need to be passed in from memory at call-time, and not use them for information that can be retrieved from precondition-creation-time callbacks. And even for things that need to come from memory at call time, we should be using ReleaseContext so we can add and remove properties without having to touch function signatures for precondition implementations that don't care about the properties we're touching. While I'm touching the Run call site, I replaced a context.TODO with a context.Background. As pointed out in the docs [1], Background is prefered for tests. [1]: https://pkg.go.dev/context#Background
1f435bd
to
ff739b2
Compare
The argument landed in b72e843 (Bug 1822844: Block z level upgrades if ClusterVersionOverridesSet set, 2020-04-30, openshift#364) for use by Upgradeable.Run. But even then, that method opens by retrieving a (possibly cached) ClusterVersion resource from the configured lister, so there's no need to pass the explicit argument. We should save explicit inputs for things that need to be passed in from memory at call-time, and not use them for information that can be retrieved from precondition-creation-time callbacks. And even for things that need to come from memory at call time, we should be using ReleaseContext so we can add and remove properties without having to touch function signatures for precondition implementations that don't care about the properties we're touching. While I'm touching the Run call site, I replaced a context.TODO with a context.Background. As pointed out in the docs [1], Background is prefered for tests. [1]: https://pkg.go.dev/context#Background
077d079
to
5469ae8
Compare
Shifting this down a level sets the stage for warn-level preconditions that can produce a non-blocking error. We don't have those yet, but once we do, we can handle aggregation within Summarize without needing further modifications to the sync-worker call-site.
There's enough going on in the RecentEtcdBackup and Upgradeable preconditions that having them in their own files doesn't feel too empty.
5469ae8
to
c77c336
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jottofar, wking 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-required Please review the full test history for this PR and help us cut down flakes. |
sandboxes are unrelated. /override ci/prow/e2e-agnostic-upgrade |
@wking: Overrode contexts on behalf of wking: ci/prow/e2e-agnostic-upgrade 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. |
@wking: all tests passed! Full PR test history. Your PR dashboard. 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. |
Assorted tidying in preparation for populating
status.history[].acceptedRisks
. No semantic changes here. Details in the commit messages.