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

pkg/reconciler/internal/conditions/conditions.go

+	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.

pkg/reconciler/reconciler.go

pkg/reconciler/reconciler_test.go

porridge

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

pkg/reconciler/reconciler.go

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

pkg/reconciler/internal/conditions/conditions.go

+	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.

pkg/reconciler/reconciler.go

pkg/reconciler/reconciler_test.go

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

pkg/reconciler/reconciler_test.go

+									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

pkg/reconciler/reconciler_test.go

+									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

pkg/reconciler/internal/conditions/conditions.go

+	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

pkg/reconciler/reconciler_test.go

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


Sorry, I cannot parse your suggestion.

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

pkg/reconciler/reconciler_test.go

+									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-actions bot 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

pkg/reconciler/reconciler.go Outdated Show resolved Hide resolved

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-01 September 7, 2022 09:58

vladbologa and 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 2985fab Compare September 15, 2022 09:41

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

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

remaining review feedback

d1c8fe0

github-actions bot 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 main Sep 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")

	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() {

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