ROX-12219: Add support for pause-reconcile annotation #29
Conversation
| ReasonUninstallSuccessful = status.ConditionReason("UninstallSuccessful") | ||
| ReasonPauseReconcileAnnotationSet = status.ConditionReason("PauseReconcileAnnotationSet") |
There was a problem hiding this comment.
| ReasonUninstallSuccessful = status.ConditionReason("UninstallSuccessful") | |
| ReasonPauseReconcileAnnotationSet = status.ConditionReason("PauseReconcileAnnotationSet") | |
| ReasonUninstallSuccessful = status.ConditionReason("UninstallSuccessful") | |
| ReasonPauseReconcileAnnotationSet = status.ConditionReason("PauseReconcileAnnotationSet") |
There was a problem hiding this comment.
nit: WDYTH renaming ReasonPauseReconcileAnnotationSet to ReasonPauseReconcileAnnotationPresent?
The Set in the end sounds to me like an action which happens.
There was a problem hiding this comment.
The only problem is that mere presence does not cause this. The value needs to be true. I renamed to ReasonPauseReconcileAnnotationTrue.
Please let me know what you think.
There was a problem hiding this comment.
FTR, this is the reason why stackrox/stackrox#2953 tests are failing.
There was a problem hiding this comment.
Good job!
@porridge my review is finished, just some nits on naming. I think your review already covers the most important parts.
| ReasonUninstallSuccessful = status.ConditionReason("UninstallSuccessful") | ||
| ReasonPauseReconcileAnnotationSet = status.ConditionReason("PauseReconcileAnnotationSet") |
There was a problem hiding this comment.
nit: WDYTH renaming ReasonPauseReconcileAnnotationSet to ReasonPauseReconcileAnnotationPresent?
The Set in the end sounds to me like an action which happens.
| Expect(err).To(BeNil()) | ||
| }) | ||
|
|
||
| By("getting the CR", func() { |
There was a problem hiding this comment.
| By("getting the CR", func() { | |
| By("getting the CR verify the reconciler is paused", func() { |
There was a problem hiding this comment.
Sorry, I cannot parse your suggestion.
| Expect(err).To(BeNil()) | ||
| }) | ||
|
|
||
| By("verifying the release is uninstalled", func() { |
There was a problem hiding this comment.
| By("verifying the release is uninstalled", func() { | |
| By("verifying reconciliation unpaused and the release is uninstalled", func() { |
There was a problem hiding this comment.
Well, we don't actually check the status here, so I'd leave this as is...
|
FTR, I changed base branch from |
porridge
force-pushed
the
vbologa/ROX-12219-pause-reconcile-annotation
branch
from
17b3f45 to
2985fab
Compare
| ReasonUninstallSuccessful = status.ConditionReason("UninstallSuccessful") | ||
| ReasonPauseReconcileAnnotationSet = status.ConditionReason("PauseReconcileAnnotationSet") |
There was a problem hiding this comment.
The only problem is that mere presence does not cause this. The value needs to be true. I renamed to ReasonPauseReconcileAnnotationTrue.
Please let me know what you think.
| Expect(err).To(BeNil()) | ||
| }) | ||
|
|
||
| By("getting the CR", func() { |
There was a problem hiding this comment.
Sorry, I cannot parse your suggestion.
| Expect(err).To(BeNil()) | ||
| }) | ||
|
|
||
| By("verifying the release is uninstalled", func() { |
There was a problem hiding this comment.
Well, we don't actually check the status here, so I'd leave this as is...
Co-authored-by: Marcin Owsiany <porridge@redhat.com>
Co-authored-by: Marcin Owsiany <porridge@redhat.com>
Co-authored-by: Marcin Owsiany <porridge@redhat.com>
Co-authored-by: Marcin Owsiany <porridge@redhat.com>
Co-authored-by: Marcin Owsiany <porridge@redhat.com>
Co-authored-by: Marcin Owsiany <porridge@redhat.com>
Co-authored-by: Marcin Owsiany <porridge@redhat.com>
Implemented support for customizable "pause-reconcile" annotation, and added unit tests.