ROX-12219: Add support for pause-reconcile annotation by vladbologa · Pull Request #29 · stackrox/helm-operator

vladbologa · 2022-09-02T10:56:06Z

Implemented support for customizable "pause-reconcile" annotation, and added unit tests.

porridge · 2022-09-05T07:50:27Z

+	ReasonUninstallSuccessful = status.ConditionReason("UninstallSuccessful")
+	ReasonPauseReconcileAnnotationSet = status.ConditionReason("PauseReconcileAnnotationSet")


Suggested change
ReasonUninstallSuccessful=status.ConditionReason("UninstallSuccessful")
ReasonPauseReconcileAnnotationSet=status.ConditionReason("PauseReconcileAnnotationSet")
ReasonUninstallSuccessful=status.ConditionReason("UninstallSuccessful")
ReasonPauseReconcileAnnotationSet=status.ConditionReason("PauseReconcileAnnotationSet")

nit: WDYTH renaming ReasonPauseReconcileAnnotationSet to ReasonPauseReconcileAnnotationPresent?
The Set in the end sounds to me like an action which happens.

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.

porridge

FTR, this is the reason why stackrox/stackrox#2953 tests are failing.

SimonBaeumer

Good job!
@porridge my review is finished, just some nits on naming. I think your review already covers the most important parts.

SimonBaeumer · 2022-09-05T14:08:56Z

+	ReasonUninstallSuccessful = status.ConditionReason("UninstallSuccessful")
+	ReasonPauseReconcileAnnotationSet = status.ConditionReason("PauseReconcileAnnotationSet")


nit: WDYTH renaming ReasonPauseReconcileAnnotationSet to ReasonPauseReconcileAnnotationPresent?
The Set in the end sounds to me like an action which happens.

SimonBaeumer · 2022-09-05T14:14:29Z

+ Expect(err).To(BeNil())
+ })
+
+ By("getting the CR", func() {


Suggested change
By("getting the CR", func() {
By("getting the CR verify the reconciler is paused", func() {

Sorry, I cannot parse your suggestion.

SimonBaeumer · 2022-09-05T14:15:32Z

+ Expect(err).To(BeNil())
+ })
+
+ By("verifying the release is uninstalled", func() {


Suggested change
By("verifying the release is uninstalled", func() {
By("verifying reconciliation unpaused and the release is uninstalled", func() {

Well, we don't actually check the status here, so I'd leave this as is...

porridge · 2022-09-07T09:59:52Z

FTR, I changed base branch from main to rebase-main-2022-09-01 in order to keep the diff sane after I force-push the backed-up main to main.
I will rebase this PR on top of the restored main next.

porridge · 2022-09-15T08:58:49Z

+	ReasonUninstallSuccessful = status.ConditionReason("UninstallSuccessful")
+	ReasonPauseReconcileAnnotationSet = status.ConditionReason("PauseReconcileAnnotationSet")


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.

porridge · 2022-09-15T09:27:16Z

+ Expect(err).To(BeNil())
+ })
+
+ By("getting the CR", func() {


Sorry, I cannot parse your suggestion.

porridge · 2022-09-15T09:28:36Z

+ Expect(err).To(BeNil())
+ })
+
+ By("verifying the release is uninstalled", func() {


Well, we don't actually check the status here, so I'd leave this as is...

Co-authored-by: Marcin Owsiany <porridge@redhat.com>

github-actionsBot added area/sdk area/testing labels Sep 2, 2022

vladbologa requested review from SimonBaeumer and porridge September 2, 2022 10:57

vladbologa mentioned this pull request Sep 2, 2022
ROX-12219: pause-reconciliation annotations support in operator stackrox/stackrox#2953
Closed
5 tasks

porridge reviewed Sep 5, 2022
View reviewed changes

porridge requested changes Sep 5, 2022
View reviewed changes

Comment threadpkg/reconciler/reconciler.go Outdated

SimonBaeumer suggested changes Sep 5, 2022
View reviewed changes

SimonBaeumer mentioned this pull request Sep 6, 2022
Upgrade Helm Operator version stackrox/stackrox#2930
Closed
1 task

porridge changed the base branch from main to rebase-main-2022-09-01September 7, 2022 09:58

vladbologaand others added 2 commits September 15, 2022 10:55

Add support for pause-reconcile annotation
7fa7b97

simple review feedback
2985fab

porridge force-pushed the vbologa/ROX-12219-pause-reconcile-annotation branch from 17b3f45 to 2985fabCompare September 15, 2022 09:41

github-actionsBot added area/deps area/docs area/infra labels Sep 15, 2022

porridge changed the base branch from rebase-main-2022-09-01 to mainSeptember 15, 2022 09:42

remaining review feedback
d1c8fe0

github-actionsBot removed area/docs area/infra area/deps labels Sep 19, 2022

porridge approved these changes Sep 19, 2022
View reviewed changes

porridge mentioned this pull request Sep 19, 2022
ROX-12219: use helm operator with pause reconcile stackrox/stackrox#3125
Merged
6 tasks

SimonBaeumer approved these changes Sep 19, 2022
View reviewed changes

porridge merged commit 89f9785 into mainSep 19, 2022

misberner pushed a commit that referenced this pull request Oct 3, 2022
ROX-12219: Add support for pause-reconcile annotation (#29)
fbf7122
Co-authored-by: Marcin Owsiany <porridge@redhat.com>

ludydoo pushed a commit that referenced this pull request Jun 27, 2023
ROX-12219: Add support for pause-reconcile annotation (#29)
ff32315
Co-authored-by: Marcin Owsiany <porridge@redhat.com>

ludydoo pushed a commit that referenced this pull request Jun 27, 2023
ROX-12219: Add support for pause-reconcile annotation (#29)
18a0fa9
Co-authored-by: Marcin Owsiany <porridge@redhat.com>

ludydoo pushed a commit that referenced this pull request Jun 27, 2023
ROX-12219: Add support for pause-reconcile annotation (#29)
5d4fc78
Co-authored-by: Marcin Owsiany <porridge@redhat.com>

ludydoo pushed a commit that referenced this pull request Jun 29, 2023
ROX-12219: Add support for pause-reconcile annotation (#29)
4219dea
Co-authored-by: Marcin Owsiany <porridge@redhat.com>

ludydoo pushed a commit that referenced this pull request Aug 4, 2023
ROX-12219: Add support for pause-reconcile annotation (#29)
650e85f
Co-authored-by: Marcin Owsiany <porridge@redhat.com>

porridge added a commit that referenced this pull request Sep 5, 2024
ROX-12219: Add support for pause-reconcile annotation (#29)
10aa91d
Co-authored-by: Marcin Owsiany <porridge@redhat.com>

porridge added a commit that referenced this pull request Feb 6, 2025
ROX-12219: Add support for pause-reconcile annotation (#29)
55ee1fa
Co-authored-by: Marcin Owsiany <porridge@redhat.com>

porridge added a commit that referenced this pull request Feb 11, 2025
ROX-12219: Add support for pause-reconcile annotation (#29)
a37a573
Co-authored-by: Marcin Owsiany <porridge@redhat.com>

porridge added a commit that referenced this pull request Feb 11, 2025
ROX-12219: Add support for pause-reconcile annotation (#29)
a0d0bc5
Co-authored-by: Marcin Owsiany <porridge@redhat.com>

porridge added a commit that referenced this pull request Feb 11, 2025
ROX-12219: Add support for pause-reconcile annotation (#29)
30a1ac3
Co-authored-by: Marcin Owsiany <porridge@redhat.com>

vladbologa mentioned this pull request Feb 24, 2025
ROX-28223: Pause-reconcile handler operator-framework/helm-operator-plugins#426
Merged

		ReasonUninstallSuccessful = status.ConditionReason("UninstallSuccessful")
		ReasonPauseReconcileAnnotationSet = status.ConditionReason("PauseReconcileAnnotationSet")

-ReasonUninstallSuccessful=status.ConditionReason("UninstallSuccessful")
-ReasonPauseReconcileAnnotationSet=status.ConditionReason("PauseReconcileAnnotationSet")
+ReasonUninstallSuccessful=status.ConditionReason("UninstallSuccessful")
+ReasonPauseReconcileAnnotationSet=status.ConditionReason("PauseReconcileAnnotationSet")

	By("getting the CR", func() {
	By("getting the CR verify the reconciler is paused", func() {

	By("verifying the release is uninstalled", func() {
	By("verifying reconciliation unpaused and the release is uninstalled", func() {

Uh oh!

Conversation

vladbologa commented Sep 2, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

porridge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SimonBaeumer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

porridge commented Sep 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vladbologa commented Sep 2, 2022•
edited
Loading