-
Notifications
You must be signed in to change notification settings - Fork 170
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
Propagate errors of ARO ImageConfig controller to ARO Operator #2939
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for the PR! There's great progress here, but this work isn't complete yet. Moving over to the base controller will now enable you to use the helper functions of the base to merge the errors of this controller into the ARO Operator status. This should happen for anything that would result in error states for this controller.
Here's an example of a call: https://github.com/Azure/ARO-RP/blob/master/pkg/operator/controllers/machineset/machineset_controller.go#L62
Additionally, as Brendan has mentioned, since the goal of this change is to propagate error cases as conditions on the operator, I'd like to see that reflected in the test cases. You can get an example of how to do that in the machineset controller tests as well: https://github.com/Azure/ARO-RP/blob/master/pkg/operator/controllers/machineset/machineset_controller_test.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My re-review was requested, but I don't see the changes suggested implemented. We should see:
- Status of the controller going to the operator, via the helpers
- Test cases that showcase how those errors manifest as conditions on the operator.
…#2939) * Propagate errors of ARO ImageConfig controller to Operator * Modified test cases
Which issue this PR addresses:
JIRA: https://issues.redhat.com/browse/ARO-3272
Fixes
What this PR does / why we need it:
Test plan for issue:
Is there any documentation that needs to be updated for this PR?
Based on changes of: #2899