📖 refactor: finalizer in CronJob example #4397
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lorenzofelletti The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
|
Hi @lorenzofelletti. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| if err := r.Get(ctx, req.NamespacedName, cronJob); err != nil { | ||
| log.Error(err, "unable to fetch CronJob") | ||
| return ctrl.Result{}, client.IgnoreNotFound(err) | ||
| } |
There was a problem hiding this comment.
Thank you for the contribution 🥇
See that:
We need to change the code under hack/docs so that when we run make generate-docs we re-scaffold all and just add the right code on top to ensure that all is always updated.
Therefore, can you please add the code there and run the command to ensure that all is fine?
There was a problem hiding this comment.
Indeed it will result in multi-version and crojob tutorial changed when you run make generate-docs
There was a problem hiding this comment.
Really thank you to looking on that, see: https://github.com/kubernetes-sigs/kubebuilder/pull/4397/files#r1859746824
| log.Info("Adding Finalizer for CronJob") | ||
| if ok := controllerutil.AddFinalizer(cronJob, myFinalizerName); !ok { | ||
| log.Error(err, "Failed to add finalizer into the custom resource") | ||
| return ctrl.Result{Requeue: true}, nil |
There was a problem hiding this comment.
would not be semantically better to error out here? For example
return ctrl.Result{}, fmt.Errorf("message")There was a problem hiding this comment.
We have no error acctually, right?
The code should create an error and then return as you suggested
@mateusoliveira43 would you like to help us by doing this change in the deploy image plugin?
There was a problem hiding this comment.
sure, will try to make changes there
| // state of the resource, and avoid the error "the object has been modified, | ||
| // please apply your changes to the latest version and try again". | ||
| if err := r.Get(ctx, req.NamespacedName, cronJob); err != nil { | ||
| log.Error(err, "unable to fetch CronJob") |
There was a problem hiding this comment.
would be better to say re-fetch here?
| // please apply your changes to the latest version and try again". | ||
| if err := r.Get(ctx, req.NamespacedName, cronJob); err != nil { | ||
| log.Error(err, "unable to fetch CronJob") | ||
| return ctrl.Result{}, client.IgnoreNotFound(err) |
There was a problem hiding this comment.
would not be better to always error out here?
There was a problem hiding this comment.
Yes you are right. We need to change it in the deploy image too.
There was a problem hiding this comment.
may I ask why? If in the meantime the resource was deleted shouldn't that be ignored?
There was a problem hiding this comment.
On top we should have : https://github.com/kubernetes-sigs/kubebuilder/blob/cbc6e383c342f1337ab37ee4aa0755957a01f9c7/testdata/project-v4-with-plugins/internal/controller/busybox_controller.go#L84C2-L99C3
If the resource is deleted, then the reconciliation stop at this point.
What we need to do is fix the tutorials to have the structure of https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4-with-plugins/internal/controller/busybox_controller.go
The changes here seems for me either incomplete if we address only the finalizer
|
Since there's a lot of rework to do on this one, I'll convert it to a draft for the moment |
k8s-ci-robot
added
the
do-not-merge/work-in-progress
k8s-ci-robot
added
size/M
| log.Error(err, "Failed to re-fetch CronJob") | ||
| return ctrl.Result{}, err | ||
| } | ||
| } |
There was a problem hiding this comment.
@lorenzofelletti Yep, that is the idea!!!
Thank you a lot
|
My bad, this PR should not be closed, but I referenced a comment from it and bot closed it @lorenzofelletti can you reopen it? |
|
don't have the permissions to re-open it haha |
Description
Refactor
CronJobsample code to follow the structure and best practices introduced by theDeployImageplugin.Motivation
Follow best practices, and provide an example that works without incurring in errors like trying to do a second update without re-fetching the resource.
References
CronJobandMultiVersionSample Code to Align withDeployImagePlugin Scaffold Structure #4291.