-
Notifications
You must be signed in to change notification settings - Fork 169
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 DNSMasqMachineConfigPool controller #2952
Conversation
8c8ee58
to
c5db0bd
Compare
pkg/operator/controllers/dnsmasq/machineconfigpool_controller_test.go
Outdated
Show resolved
Hide resolved
c5db0bd
to
7f01f35
Compare
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.
LGTM - one nit.
ctx := context.Background() | ||
_, err := r.Reconcile(context.Background(), tt.request) |
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.
nit: Same as the other PR - I'd like to see either ctx
or context.Background()
instead of this hybrid.
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.
LGTM. I had the same nit as Brendan
ecc2e97
7f01f35
to
ecc2e97
Compare
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.
LGTM
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.
LGTM
Propagate errors of ARO DNSMasqMachineConfigPool controller to ARO Operator
Jira: https://issues.redhat.com/browse/ARO-3255
Which issue this PR addresses:
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?