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

WithFinalizer #228

Merged
merged 2 commits into from May 9, 2022
Merged

WithFinalizer #228

merged 2 commits into from May 9, 2022

Conversation

scothis
Copy link
Contributor

@scothis scothis commented May 6, 2022

WithFinalizer is a SubReconciler that allows external state to be
allocated and then cleaned up once the parent resource is deleted. When
the parent resource is not terminating, the finalizer is set on the
parent resource before the nested reconciler is called. When the parent
resource is terminating, the finalizer is cleared only after the nested
reconciler returns without an error.

func SyncExternalState() *reconcilers.SubReconciler {
	return &reconcilers.WithFinalizer{
		Finalizer: "unique.finalizer.name"
		Reconciler: &reconcilers.SyncReconciler{
			Sync: func(ctx context.Context, parent *resources.TestResource) error {
				// allocate external state
				return nil
			},
			Finalize: func(ctx context.Context, parent *resources.TestResource) error {
				// cleanup the external state
				return nil
			},
		},
	}
}

Signed-off-by: Scott Andrews andrewssc@vmware.com

WithFinalizer is a SubReconciler that allows external state to be
allocated and then cleaned up once the parent resource is deleted. When
the parent resource is not terminating, the finalizer is set on the
parent resource before the nested reconciler is called. When the parent
resource is terminating, the finalizer is cleared only after the nested
reconciler returns without an error.

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
@codecov
Copy link

codecov bot commented May 6, 2022

Codecov Report

Merging #228 (a9ce88c) into main (af55ebc) will increase coverage by 1.52%.
The diff coverage is 80.64%.

@@            Coverage Diff             @@
##             main     #228      +/-   ##
==========================================
+ Coverage   65.07%   66.60%   +1.52%     
==========================================
  Files           9        9              
  Lines        1008     1099      +91     
==========================================
+ Hits          656      732      +76     
- Misses        330      345      +15     
  Partials       22       22              
Impacted Files Coverage Δ
reconcilers/reconcilers.go 81.91% <80.64%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af55ebc...a9ce88c. Read the comment docs.

Copy link
Collaborator

@mamachanko mamachanko left a comment

Choose a reason for hiding this comment

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

Given that finalizers can be tricky, I could imagine it to be helpful if there’re a few words of guidance when to choose WithFinalizer over Sync/ChildReconciler with Finalize. I would find convenience or expressiveness or improved composability ensuring for the adopter.

Nice, @scothis!

@@ -342,6 +343,36 @@ func SwapRESTConfig(rc *rest.Config) *reconcilers.SubReconciler {
}
```

#### WithFinalizer

[`WithFinalizer`](https://pkg.go.dev/github.com/vmware-labs/reconciler-runtime/reconcilers#WithFinalizer) allows external state to be allocated and then cleaned up once the parent resource is deleted. When the parent resource is not terminating, the finalizer is set on the parent resource before the nested reconciler is called. When the parent resource is terminating, the finalizer is cleared only after the nested reconciler returns without an error.
Copy link
Collaborator

Choose a reason for hiding this comment

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

[suggestion]: Can you motivate usage of a WithFinalizer subreconciler over a ChildReconciler/SyncRenciler with Finalize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a note under the finalizers section. We can continue to iterate on it

Signed-off-by: Scott Andrews <andrewssc@vmware.com>
@scothis scothis merged commit da4c0ef into vmware-labs:main May 9, 2022
@scothis scothis deleted the with-finalizer branch May 9, 2022 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants