-
Notifications
You must be signed in to change notification settings - Fork 545
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
feat(operator): adopt referenced installplans #1661
feat(operator): adopt referenced installplans #1661
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: njhale 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 |
Is this at all impacted by #1669 |
It shouldn't be. This adopts the latest |
/retest |
ip := &operatorsv1alpha1.InstallPlan{} | ||
ip.SetNamespace(ref.Namespace) | ||
ip.SetName(ref.Name) | ||
if err := r.adopt(ctx, operator, ip); err != nil { |
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.
Am I missing where the installplan would be "unadopted" when it is no longer the current one?
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.
Nope, we don't disown InstallPlans. Is there a reason to do so now that we have a cap on the number that can exist at a given time?
/retest |
2660822
to
2c0d0fc
Compare
/retest |
2 similar comments
/retest |
/retest |
// TODO(njhale): parallelize | ||
var ( | ||
errs []error | ||
mu sync.Mutex |
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.
why not an error channel instead of a mutex + err slice?
Adopt InstallPlans to the Operator(s) represented by the Subscriptions that reference them. This means that InstallPlans are components of the operators they install.
/lgtm |
This PR failed tests for 1 times with 7 individual failed tests and 4 skipped tests. A test is considered flaky if failed on multiple commits. totaltestcount: 1
|
/retest Please review the full test history for this PR and help us cut down flakes. |
This PR failed tests for 1 times with 5 individual failed tests and 4 skipped tests. A test is considered flaky if failed on multiple commits. totaltestcount: 1
|
/retest Please review the full test history for this PR and help us cut down flakes. |
/test e2e-aws-olm |
/hold seeing this test fail locally as well, investigating |
Allow a user defined SA e2e test to pass if ultimate step status is Present as well as Created. Both states are roughly equivalent for the purposes of the test. The state most likely hits Present instead of Created in some cases when the resourceVersion of the InstallPlan changes between the start and end of plan execution, causing the next reconciliation to think the resources already existed. This change can be reverted if we use a status patch in place of an update.
/retest Please review the full test history for this PR and help us cut down flakes. |
21 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/hold Checking this out locally. |
Fix merged, let's try this again. /hold cancel |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
Adopt InstallPlans to the Operator(s) represented by the Subscriptions
that reference them. This means that InstallPlans are components of the
operators they install.