Propagate errors of ARO MachineHealthCheckController to ARO Operator #3177
Conversation
There was a problem hiding this comment.
Looks good so far, thanks for your work on this. One bug to fix. The last thing I think is to add some E2E tests for validating the conditions of the operator during each test execution.
As a final super-nit, there are quite a few newlines added that I don't think add value (at least 6). It's likely best to delete those to reduce the diff.
| err := r.client.Get(ctx, types.NamespacedName{Name: arov1alpha1.SingletonClusterName}, instance) | ||
| // Reconcile watches MachineHealthCheck objects, and if any changes, | ||
| // reconciles the associated ARO MachineHealthCheck object | ||
|
|
There was a problem hiding this comment.
I'm not 100% sure, but I think this extra newline might mess up godoc on this function.
There was a problem hiding this comment.
So should I just remove the blank lines or the comment also? Because other ones are just comments on it for code readability. I can remove them anyways we have a proper doc on those available under readme.md.
pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go
Show resolved
Hide resolved
safwank97
dismissed
SudoBrendan’s stale review
Addressed all coments just left with E2E
pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go
Outdated
Show resolved
Hide resolved
pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go
Outdated
Show resolved
Hide resolved
|
Removed the MHC Prefix. |
|
Done. |
pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go
Outdated
Show resolved
Hide resolved
|
I'll take care of that part you mentioned. Thank you very much. |
pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go
Outdated
Show resolved
Hide resolved
pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go
Outdated
Show resolved
Hide resolved
pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go
Outdated
Show resolved
Hide resolved
safwank97
requested review from
jewzaam,
bennerv,
hawkowl,
rogbas,
petrkotas,
jharrington22,
cblecker,
facchettos,
UlrichSchlueter and
s-amann
as code owners
safwank97
requested review from
Shivkumar13,
yjst2012,
anshulvermapatel and
hlipsig
as code owners
pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go
Outdated
Show resolved
Hide resolved
pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go
Outdated
Show resolved
Hide resolved
…per message for setProgressing use case
safwank97
force-pushed
the
propogate-errors-mhc-controller
branch
from
df83a40
to
bf8b8d9
Compare
|
Done. |
| if !instance.Spec.OperatorFlags.GetSimpleBoolean(managed) { | ||
| r.SetProgressing(ctx, "Not MHC Managed for cluster maintenance purpose.") |
There was a problem hiding this comment.
Leaving a comment here based on our discussion yesterday, it seems like Progressing isn't necessary here. We should remove the SetProgresing and ClearProgressing calls.
There was a problem hiding this comment.
Agree here. Progressing is not required and will solve a few of the other requested changes on this PR by removing it.
There was a problem hiding this comment.
Removed those calls.
…zure#3177) * Propagate errors of ARO MachineHealthCheckController to ARO Operator
Which issue this PR addresses:
Fixes - https://issues.redhat.com/browse/ARO-3256
What this PR does / why we need it:
Aggregates errors from the MachineHealthCheck controller and propagates to the ARO Operator.
Test plan for issue:
As of now I think it must pass the local test cases after spinning up a test cluster and if case scenarios falls under the test conditions defined in code then error propagation will happen.
Is there any documentation that needs to be updated for this PR?
None that I'm aware of.