fix(sub): Reset ResolutionFailed cond when error is resolved #2296
Conversation
anik120
force-pushed
the
reset-sub-condition
branch
from
8345b25
to
1ccaab9
Compare
anik120
force-pushed
the
reset-sub-condition
branch
2 times, most recently
from
c605f28
to
dc5bae5
Compare
anik120
force-pushed
the
reset-sub-condition
branch
2 times, most recently
from
49f6ef6
to
79eae9c
Compare
anik120
force-pushed
the
reset-sub-condition
branch
5 times, most recently
from
36d140b
to
d7563eb
Compare
In operator-framework#2269, a new condition was introduced for Subscription to indicate any dependency resolution error in it's message. However, when the case of the error was resolved, the condition status (true) and the message was sticking around. This PR fixes the issue, and makes sure that the condition status is set to false, and the message and reason of the condition are cleared off. Signed-off-by: Anik Bhattacharjee <anikbhattacharya93@gmail.com>
…'s own task Signed-off-by: Anik Bhattacharjee <anikbhattacharya93@gmail.com>
anik120
force-pushed
the
reset-sub-condition
branch
from
d7563eb
to
ac88ff0
Compare
The statuses of the subscriptions are updated multiple number of times while syncing the resolving namespace. This commit switches to preserving the state of the subscription statues instead, and only updating the statuses on cluster only when it's neccessary. Signed-off-by: Anik Bhattacharjee <anikbhattacharya93@gmail.com>
anik120
force-pushed
the
reset-sub-condition
branch
from
ac88ff0
to
e419a97
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anik120, benluddy 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 |
openshift-ci
bot
added
the
approved
| _, err = o.client.OperatorsV1alpha1().Subscriptions(s.Namespace).UpdateStatus(context.TODO(), latest, updateOpts) | ||
|
|
||
| latest.Status = sub.Status | ||
| sub = *latest |
There was a problem hiding this comment.
I might be misunderstanding the intent here, but based on https://github.com/operator-framework/operator-lifecycle-manager/pull/2296/files#r678478765, it seems like you're trying to make sure that subs has the latest version of this particular sub in it.
This line seems like it only changes the sub that is scoped to the goroutine defined starting in line 1275. It seems like if you're trying to update the sub variable defined in line 1273, you should either:
- change the name of
subat line 1275 to avoid shadowingsubat line 1273, OR - add an index variable for the for loop, pass that to the goroutine, and then set
subs[i] = latest
There was a problem hiding this comment.
@joelanford you're right that the intent is to make sure subs has the latest version of the particular sub in it. And also, the sub inside the goroutine is what's being changed here. But if you notice in L1294, it's the individual sub from subs that's being passed to the goroutines, so it's effectively achieving the same objective as what the options you laid out would.
Signed-off-by: Anik Bhattacharjee <anikbhattacharya93@gmail.com>
anik120
force-pushed
the
reset-sub-condition
branch
from
b975d34
to
8aadfc7
Compare
Signed-off-by: Anik Bhattacharjee <anikbhattacharya93@gmail.com>
anik120
force-pushed
the
reset-sub-condition
branch
from
8aadfc7
to
17d779c
Compare
|
/lgtm |
Description of the change:
In #2269, a new condition was introduced for Subscription to indicate any
dependency resolution error in it's message. However, when the case of
the error was resolved, the condition status (true) and the message
was sticking around. This PR fixes the issue, and makes sure that the
condition status is set to false, and the message and reason of the
condition are cleared off.
Motivation for the change:
Reviewer Checklist
/doc