Add OwnerReference to generated Jobs

[ChristianCiach]

Since https://github.com/stakater/Reloader/pull/486 Reloader is able to trigger Jobs from existing CronJob, which is awesome :tada:

But I noticed that the generated Jobs are missing owner references to the CronJob that was used to create them. This creates some issues:

• The created jobs are not deleted when the CronJob is deleted.
• The created jobs are not displayed in tools like ArgoCD or K9s.

Prior art:

• kubectl: https://github.com/kubernetes/kubectl/blob/5e1a193aef164bbd16b770684ff7711c6beed7d4/pkg/cmd/create/create_job.go#L268
• Kubernetes Dashboard: https://github.com/kubernetes/dashboard/blob/ee8b965861da0e6e6c3c764b91b7824f92383e15/modules/api/pkg/resource/cronjob/jobs.go#L121
• K9s: https://github.com/derailed/k9s/blob/39eef03373871388fc3cf2fa434d055e1eb97319/internal/dao/cronjob.go#L80
• ArgoCD: https://github.com/argoproj/argo-cd/blob/fe598a831e42a2838242f99d6a74be520f27908a/resource_customizations/batch/CronJob/actions/create-job/action.lua#L46

I successfully tested this change in one of our clusters.

[ChristianCiach]

I've added a commit that adds the annotation cronjob.kubernetes.io/instantiate: manual to jobs that have been triggered by Reloader. This seems to be a convention, because every other tool mentioned above (except for K9s) adds this annotation to manually scheduled jobs that have been created from cronjobs. I think this annotation is necessary to avoid a warning from the cronjob controller if it sees a job that has been created by another controller.

[ChristianCiach]

Is there something left to do? I see that some pipeline job failed, but I believe this has nothing to do with my changes.

[ChristianCiach]

Thank you for reviewing! I will add the requested test cases sometime this week.

[ChristianCiach]

I am sorry, I don't understand what kind of tests I should add. All the existing tests only check if the called function (in this case CreateJobFromCronjob) returns an error or not, and this seemed to be good enough for all previous PRs (including the one that added the cronjob-feature to Reloader). The existing test TestCreateJobFromCronjob still checks if CreateJobFromCronjob returns an error, and this implicitly includes any changes I made to CreateJobFromCronjob.

I am fine with adding any test case you like, but I would kindly ask for some more guidance regarding what exactly you want me to add. Looking at the existing test cases, it doesn't seem like other contributors where ever asked to test anything beyond "Please ensure that the function doesn't return an error".

By the way, there is surely no need to change TestReCreateJobFromJob, because my PR didn't touch ReCreateJobFromJob in any way.

[msafwankarim]

Sorry for not being clear enough. When writing the comment I imagined having some kind of validation to check if owner reference was added or not. This is just to make sure that in future if this function gets modified then we have a test case in place to verify that this owner reference feature doesn't get vanished unintentionally. Before this change, having an error check was enough because that was the only area where there was a margin for a failing case.

And sorry no need to change TestReCreateJobFromJob. I misread the code during the review and though that it was re creating job from the cronjob.

Please let me know if there is any confusion.

[ChristianCiach]

Understood :) I've added a little check that ensures the presence of a controller: true ownerReference that references the kind and name of the owner CronJob. Let me know what you think!

I am sorry if the code isn't very Go'ish. I am an experienced developer, but I am new to Go and its conventions.

[ChristianCiach]

Oops, forgot to remove the Println. Will clean up!

Edit: Now there are conflicts, weird. Gimme a minute...

[ChristianCiach]

There you go, please take a look :)