API-1835: Don't use Update in removestaleconditionscontroller #1864
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
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto 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
bertinatto
force-pushed
the
try-apply-stale-conditions
branch
from
5b6845e
to
f63e72e
Compare
bertinatto
changed the title
| conditions []string | ||
| operatorClient v1helpers.OperatorClient | ||
| controllerInstanceName string | ||
| conditions []string |
There was a problem hiding this comment.
please rename to conditionTypes or even conditionTypesToRemove
There was a problem hiding this comment.
Renamed to conditionTypesToRemove
| } | ||
| return factory.New(). | ||
| ResyncEvery(time.Minute). | ||
| WithSync(c.sync). | ||
| WithInformers(operatorClient.Informer()). | ||
| ToController( | ||
| "RemoveStaleConditionsController", // don't change what is passed here unless you also remove the old FooDegraded condition | ||
| c.controllerInstanceName, | ||
| eventRecorder.WithComponentSuffix("remove-stale-conditions"), | ||
| ) | ||
| } | ||
|
|
||
| func (c RemoveStaleConditionsController) sync(ctx context.Context, syncContext factory.SyncContext) error { |
There was a problem hiding this comment.
would you mind adding some unit tests for the sync function ?
There was a problem hiding this comment.
Added some unit tests (bases on the node status controller)
| var removedCount int | ||
| jsonPatch := jsonpatch.New() | ||
| for i, existingCondition := range operatorStatus.Conditions { | ||
| for _, conditionToRemove := range c.conditions { |
There was a problem hiding this comment.
please rename to conditionTypeToRemove
| } | ||
| jsonPatch.WithRemove( | ||
| fmt.Sprintf("/status/conditions/%d", removeAtIndex), | ||
| jsonpatch.NewTestCondition(fmt.Sprintf("/status/conditions/%d/type", removeAtIndex), conditionToRemove), |
There was a problem hiding this comment.
is there an e2e test that we could rely on ?
bertinatto
force-pushed
the
try-apply-stale-conditions
branch
from
f63e72e
to
cc91110
Compare
bertinatto
force-pushed
the
try-apply-stale-conditions
branch
from
cc91110
to
8f287dd
Compare
|
@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. |
|
@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. |
|
@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. |
Proof: openshift/cluster-authentication-operator#729
Manual testing done: