Skip to content

Cronjob example - add fianlizer #4564

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.

Sign up for GitHub

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

Jump to bottom
Open
matrus2 opened this issue Feb 14, 2025 · 4 comments · May be fixed by #4789
Open

Cronjob example - add fianlizer #4564

matrus2 opened this issue Feb 14, 2025 · 4 comments · May be fixed by #4789
Assignees
@matrus2
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@matrus2
Copy link

matrus2 commented Feb 14, 2025

What do you want to happen?

I wonder why in the cronjob example there is a condition to check if cronjob contains finalizer and if so add it. See:

		if !controllerutil.ContainsFinalizer(cronJob, myFinalizerName) {
			controllerutil.AddFinalizer(cronJob, myFinalizerName)

kubebuilder/docs/book/src/cronjob-tutorial/testdata/finalizer_example.go

Line 69 in 93a0823

if !controllerutil.ContainsFinalizer(cronJob, myFinalizerName) {

However ContainsFinalizer seems to be overkill as AddFinalizer function do the same check inside and it will not add the finalizer once it is already there:

func AddFinalizer(o client.Object, finalizer string) (finalizersUpdated bool) {
	f := o.GetFinalizers()
	for _, e := range f {
		if e == finalizer {
			return false
		}
	}
	o.SetFinalizers(append(f, finalizer))
	return true
}

I would like to remove ContainsFinalizer as it is not needed and update documentation. Before doing so please comment if my reasoning is fine.

Extra Labels

No response
@matrus2 matrus2 added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 14, 2025
@matrus2
Copy link
Author

matrus2 commented Feb 14, 2025

/assign

@mateusoliveira43
Copy link

would suggest a further refactor

AddFinalizer returns a bool telling if the object was updated or not. We could use it to call Update or not

Example idea

                updated := controllerutil.AddFinalizer(cronJob, myFinalizerName)
		if updated {
			if err := r.Update(ctx, cronJob); err != nil {
				return ctrl.Result{}, err
			}
		}

@camilamacedo86
Copy link
Member

The CronJob examples should follow the same design approach as the Deploy Image plugin. That means they should be scaffolded in a similar way, with modifications to implement CronJob-specific logic instead of managing an image.

For reference, see this example:
🔗 BusyBox Controller Example

We have issues tracking this effort.

Could you also review the Deploy Image implementation? If you think any improvements should be made there, we should apply them consistently.

Feel free to raise PRs with your suggestions—we can iterate from there! 🚀 If you spot any areas that could be improved, go for it!

Thank you for all your contributions! 🥇

@matrus2 matrus2 linked a pull request Apr 25, 2025 that will close this issue
Open
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue as fresh with /remove-lifecycle stale
  • Close this issue with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@matrus2 matrus2

Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

🐛 remove superfluous addFinalizer error handling ✨ matrus2/kubebuilder
5 participants
@camilamacedo86 @matrus2 @k8s-ci-robot @k8s-triage-robot @mateusoliveira43