Skip to content
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

Remove the sync finalizer #369

Merged
merged 2 commits into from Dec 30, 2019

Conversation

jackkleeman
Copy link
Contributor

Fixes #352. Removes the sync finalizer on objects that have been cached,
and the various bits of logic around keeping track of what objects have
finalizers and removing them on shutdown etc.

@jackkleeman jackkleeman force-pushed the remove-sync-finalizer branch 4 times, most recently from 945e2df to 39ba9db Compare December 23, 2019 12:42
Fixes open-policy-agent#352. Removes the sync finalizer on objects that have been cached,
and the various bits of logic around keeping track of what objects have
finalizers and removing them on shutdown etc.

Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking care of this! A couple of changes needed.

Apologies if there is a slow review cycle this week, as I'm on vacation.

@@ -153,7 +156,6 @@ func (r *ReconcileConfig) Reconcile(request reconcile.Request) (reconcile.Result
}

newSyncOnly := newSet()
toClean := newSet()
if instance.GetDeletionTimestamp().IsZero() {
if !hasFinalizer(instance) {
instance.SetFinalizers(append(instance.GetFinalizers(), finalizerName))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove this finalizer too. Let's leave that for a follow-on PR in the interest of incremental changes.

pkg/controller/sync/sync_controller.go Show resolved Hide resolved
pkg/controller/sync/sync_controller.go Show resolved Hide resolved
pkg/controller/sync/sync_controller.go Show resolved Hide resolved
test/bats/test.bats Show resolved Hide resolved
@jackkleeman jackkleeman force-pushed the remove-sync-finalizer branch 3 times, most recently from 1c2eed6 to 307e5b5 Compare December 30, 2019 11:28
Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@maxsmythe maxsmythe merged commit f7494cb into open-policy-agent:master Dec 30, 2019
@jackkleeman jackkleeman deleted the remove-sync-finalizer branch January 2, 2020 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do we need finalizers on synced resources?
2 participants