API-1835: Migrate WellKnownReadyController to SSA #705
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bertinatto: This pull request references API-1835 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.18.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold |
bertinatto
force-pushed
the
try-apply-wellknown-controller
branch
2 times, most recently
from
af92124
to
94d605e
Compare
|
/retest-required |
bertinatto
force-pushed
the
try-apply-wellknown-controller
branch
2 times, most recently
from
beab95d
to
881a669
Compare
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest |
|
/test all |
|
/test e2e-operator |
|
@bertinatto: This pull request references API-1835 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.18.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
| progressing, | ||
| } | ||
| for _, condition := range conditions { | ||
| if condition != nil { |
There was a problem hiding this comment.
what happens to a condition that is nil ? I think it will be removed. Is this what we want :) ?
There was a problem hiding this comment.
IIUC it won't be removed because the field is a listType=map. Based on this, we should only update the condition we are specifically interested in and leave the other existing conditions untouched.
This logic (only apply valid, non-nil conditions) was introduced to align with the current behavior. For example, imagine get into this branch:
Under the current behavior, only the available condition is set, the progressing condition is ignored.
I did try to set the progressing condition unconditionally, but that's not what CI expects.
Does that make sense?
There was a problem hiding this comment.
with SSA and listType=map, the behavior goes liek this
- SSA for fieldmanager/one that sets condition/foo and condition/bar
- objectect has condition/foo and condition/bar
- SSA for fieldmanager/one, sets condition/bar
- object has condition/bar, condition/foo is removed
I suspect we wnt to always set available adn progressing.
There was a problem hiding this comment.
hm, what if step 3 is:
- SSA for fieldmanager/two sets condition/baz
then step 4 would be:
- object has condition/bar, condition/foo and condition/baz?
is that correct?
There was a problem hiding this comment.
is that correct?
Yes. The fieldmanager/2 doesn't manipulate the conditions provided by fieldmanager/1
There was a problem hiding this comment.
@bertinatto are you willing to check the controls that have already been changed and make sure they don't have this issue?
bertinatto
force-pushed
the
try-apply-wellknown-controller
branch
2 times, most recently
from
0d0821a
to
b8a27c0
Compare
|
/retest |
| WithStatus(operatorv1.ConditionFalse) | ||
| progressing := applyoperatorv1.OperatorCondition(). | ||
| WithType(common.ControllerProgressingConditionName(controllerName)). | ||
| WithStatus(operatorv1.ConditionTrue) |
There was a problem hiding this comment.
This condition will be changed when the the code enters line 138. It might be changed from progressing=false to progressing=true, not ideal. Do we care ?
I'm just wondering if, in this case, it would be better to read the previously applied condition and use it.
Thoughts ?
|
/retest |
bertinatto
force-pushed
the
try-apply-wellknown-controller
branch
from
b8a27c0
to
fd3b5d5
Compare
|
/retest |
|
required followups, but not fatal /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto, deads2k 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
|
/override ci/prow/e2e-operator |
|
@deads2k: Overrode contexts on behalf of deads2k: ci/prow/e2e-operator 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-sigs/prow repository. |
|
@bertinatto: 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-sigs/prow repository. I understand the commands that are listed here. |
|
[ART PR BUILD NOTIFIER] Distgit: ose-cluster-authentication-operator |
This is a follow-up requirements from deads2k: openshift#705 (comment)
Compared e2e-operator job from #709 with the same job from this PR: