Clear (existing) error cond from Subscription, once error resolved #3166
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anik120
force-pushed
the
catalog-recreate-clear-resolution-error
branch
3 times, most recently
from
a4ffc39 to
3227583
Compare
|
/retest |
m1kola
reviewed
Feb 1, 2024
|
/retest |
anik120
force-pushed
the
catalog-recreate-clear-resolution-error
branch
from
3227583 to
f8c215b
Compare
|
Generally looks good to me. But @m1kola has some quyestions. |
@tmshort there was a change I needed to do in the e2e test to reflect "BundleUnpacking cond is now removed instead of being set to false" in the e2e test 😅 Made that change, all tests should pass now. |
The func `removeSubsCond` takes in a list of pointers to Subscription objects, modifies the objects that the pointers point to, but return a new list of those pointers. A [PR](operator-framework#2942) included in the v0.25.0 release [changed the way the output of that function was being used](https://github.com/operator-framework/operator-lifecycle-manager/pull/2942/files#diff-a1760d9b7ac1e93734eea675d8d8938c96a50e995434b163c6f77c91bace9990R1146-R1155) leading to a regression. This PR fixes the `removeSubsCond` function, fixing the regression as a result. Closes operator-framework#3162 Signed-off-by: Anik Bhattacharjee <[email protected]>
anik120
force-pushed
the
catalog-recreate-clear-resolution-error
branch
from
f8c215b to
0cb11cb
Compare
|
/lgtm |
openshift-ci
bot
added
lgtm
m1kola
approved these changes
Feb 2, 2024
| Type: v1alpha1.SubscriptionBundleUnpacking, | ||
| Status: corev1.ConditionFalse, | ||
| }) | ||
| o.removeSubsCond(subs, v1alpha1.SubscriptionBundleUnpacking) |
There was a problem hiding this comment.
If we want to keep the condition, I think alternative fix will be something like this:
Suggested change
| o.removeSubsCond(subs, v1alpha1.SubscriptionBundleUnpacking) | |
| o.setSubsCond(subs, v1alpha1.SubscriptionCondition{ | |
| Type: v1alpha1.SubscriptionBundleUnpacking, | |
| Status: corev1.ConditionFalse, | |
| }) |
Because setSubsCond returns a new slice of pointers containing updated subs only (similar to old implementation of removeSubsCond).
There was a problem hiding this comment.
Removing condition can potentially break clients, but given the fact:
- It is relatively recent change
- And not consistent with other conditions
I think it might be ok to remove it.
There was a problem hiding this comment.
My vote would be to remove it. Reasons I say that are:
- When I asked around I wasn't able to get a "a user asked for it and hence it was added in there" answer.
- Which feeds into the doubts about the usefulness of clients needing to know if BundleUnpacking is true or false. As opposed to for example, the usefulness of clients needing to know if
ResolutionFailedis true or false.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anik120, m1kola, tmshort 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 |
anik120
added a commit
to anik120/operator-framework-olm
that referenced
this pull request
Feb 2, 2024
…ror resolved Upstream fix: operator-framework/operator-lifecycle-manager#3166 Upstream-repository: operator-lifecycle-manager Upstream-commit: 54da66a9996632315827ba37e14823de6405b4d9 Signed-off-by: Anik Bhattacharjee <[email protected]>
openshift-ci
bot
added
the
qe-approved
anik120
added a commit
to anik120/operator-lifecycle-manager
that referenced
this pull request
Feb 5, 2024
However, this test is skipped for upstream, and runs only downstream. This PR is a follow up to operator-framework#3166 to reflect the `BundleUnpacking` condition removal in the remaining test.
anik120
added a commit
to anik120/operator-lifecycle-manager
that referenced
this pull request
Feb 5, 2024
PR operator-framework#3166 removes the `BundleUnpacking` condition once resolution is successful. PR operator-framework#3166 [modified an e2e test](operator-framework@54da66a#diff-11f70fc71ac22d725767916f562789de88d06eb9ebe19f337a59fd7035a3ca2dR2448) to reflect that change. However, the test being fixed in this PR is skipped for upstream, and runs only downstream.This PR is a follow up to operator-framework#3166 to reflect the `BundleUnpacking` condition removal in the remaining test.
11 tasks
anik120
added a commit
to anik120/operator-lifecycle-manager
that referenced
this pull request
Feb 5, 2024
PR operator-framework#3166 removes the `BundleUnpacking` condition once resolution is successful. PR operator-framework#3166 [modified an e2e test](operator-framework@54da66a#diff-11f70fc71ac22d725767916f562789de88d06eb9ebe19f337a59fd7035a3ca2dR2448) to reflect that change. However, the test being fixed in this PR is skipped for upstream, and runs only downstream.This PR is a follow up to operator-framework#3166 to reflect the `BundleUnpacking` condition removal in the remaining test. Signed-off-by: Anik Bhattacharjee <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 5, 2024
PR #3166 removes the `BundleUnpacking` condition once resolution is successful. PR #3166 [modified an e2e test](54da66a#diff-11f70fc71ac22d725767916f562789de88d06eb9ebe19f337a59fd7035a3ca2dR2448) to reflect that change. However, the test being fixed in this PR is skipped for upstream, and runs only downstream.This PR is a follow up to #3166 to reflect the `BundleUnpacking` condition removal in the remaining test. Signed-off-by: Anik Bhattacharjee <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the change:
The func
removeSubsCondtakes in a list of pointers to Subscription objects, modifies theobjects that the pointers point to, but return a new list of those pointers. A PR included in
the v0.25.0 release changed the way the output of that function was being used leading to a regression.
This PR fixes the
removeSubsCondfunction, fixing the regression as a result.Motivation for the change:
Closes #3162
Reviewer Checklist
/doc[FLAKE]are truly flaky and have an issue