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

Cronjob example - add fianlizer #4564

Open
matrus2 opened this issue Feb 14, 2025 · 3 comments
Open

Cronjob example - add fianlizer #4564

matrus2 opened this issue Feb 14, 2025 · 3 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

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)

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
Contributor

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! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

3 participants