Skip to content
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

move logic for managing disabled state back inside the reconciler #69

Merged
merged 1 commit into from Nov 30, 2020

Conversation

dhellmann
Copy link
Contributor

Avoid duplicating the logic for handling the disabled state by moving
the call to initialize the ClusterOperator from main into
SetupWithManager. That also lets us reuse the existing reconciler
method to set the ClusterOperator status, so we can eliminate
SetCOInDisabledState entirely.

This is a follow-up to #63

/cc @sadasu @asalkeld

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 12, 2020
Avoid duplicating the logic for handling the disabled state by moving
the call to initialize the ClusterOperator from main into
SetupWithManager. That also lets us reuse the existing reconciler
method to set the ClusterOperator status, so we can eliminate
SetCOInDisabledState entirely.

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>
@sadasu
Copy link
Contributor

sadasu commented Nov 12, 2020

+1 for eliminating 2 separate ways to set the CO in Disabled state. Will let @asalkeld take a look also.

@sadasu
Copy link
Contributor

sadasu commented Nov 13, 2020

e2e-agnostic failure not related to this change.

/test e2e-agnostic

@sadasu
Copy link
Contributor

sadasu commented Nov 14, 2020

/test e2e-agnostic

@dhellmann
Copy link
Contributor Author

/retest

@sadasu
Copy link
Contributor

sadasu commented Nov 16, 2020

/test e2e-agnostic

3 similar comments
@sadasu
Copy link
Contributor

sadasu commented Nov 17, 2020

/test e2e-agnostic

@sadasu
Copy link
Contributor

sadasu commented Nov 17, 2020

/test e2e-agnostic

@sadasu
Copy link
Contributor

sadasu commented Nov 18, 2020

/test e2e-agnostic

Copy link
Contributor

@sadasu sadasu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, sadasu

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 18, 2020
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

Copy link
Contributor

@asalkeld asalkeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

thanks! sorry for the slow review

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@sadasu
Copy link
Contributor

sadasu commented Nov 19, 2020

/test e2e-agnostic

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

19 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@sadasu
Copy link
Contributor

sadasu commented Nov 29, 2020

/hold
Will remove hold after CIs start passing again.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 29, 2020
@sadasu
Copy link
Contributor

sadasu commented Nov 30, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2020
@stbenjam
Copy link
Member

We are well past the point where CBO is relevant for e2e-agnostic. I'm comfortable overriding this for now, but let's make sure we're looking at the CI logs before we do that on any others.

/override ci/prow/e2e-agnostic

@openshift-ci-robot
Copy link
Contributor

@stbenjam: Cannot update PR status for context ci/prow/e2e-agnostic

In response to this:

We are well past the point where CBO is relevant for e2e-agnostic. I'm comfortable overriding this for now, but let's make sure we're looking at the CI logs before we do that on any others.

/override ci/prow/e2e-agnostic

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/test-infra repository.

@openshift-ci-robot
Copy link
Contributor

@stbenjam: Overrode contexts on behalf of stbenjam: ci/prow/e2e-agnostic

In response to this:

We are well past the point where CBO is relevant for e2e-agnostic. I'm comfortable overriding this for now, but let's make sure we're looking at the CI logs before we do that on any others.

/override ci/prow/e2e-agnostic

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/test-infra repository.

@openshift-merge-robot openshift-merge-robot merged commit 675f8bf into openshift:master Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants