annotations for the job object of a scaledjob

[InbarRose]

Proposal

i would like to be able to specify annotations which should be applied to each job created by the scaledjob.
currently, the job is only getting the labels from the scaledjob.
I am able to specify annotations for the pod, but not for the job.

Use-Case

i need to annotate the jobs in my cluster

Is this a feature you are interested in implementing yourself?

Maybe

Anything else?

this PR did the same thing for labels: https://github.com/kedacore/keda/pull/1322/files#diff-543511c1a0b5667aed68231a662b053e211fa61392ab63f8b87e7b3d45faf770

I propose a similar fix for annotations.

[JorTurFer]

This totally makes sense IMO
Are you willing to contribute with the feature?

[InbarRose]

sure, what is required?

[JorTurFer]

I reckon that something similar to the commit that you have linked. I mean, the code will be quite similar but using annotations instead of labels, and the changelog update.
Maybe a unit test would be nice, but that's all in this case as doc changes aren't required here

[zroubalik]

Absolutely valid request and I think that it should be documented as well

[JorTurFer]

I think that it should be documented as well

How would you document it? I mean, the only option that I see is adding a note in ScaledJob section

[zroubalik]

I think that it should be documented as well

How would you document it? I mean, the only option that I see is adding a note in ScaledJob section

yeah, https://keda.sh/docs/2.10/concepts/scaling-jobs/#scaledjob-spec

[JorTurFer]

@InbarRose , the note in docs is also required :)

[InbarRose]

@InbarRose , the note in docs is also required :)

I am not sure where to put the note, as there is no indication in the docs about labels either.

[zroubalik]

I would put some labels and annotation to the metada of the Spec, with a comment that these get applied to the job as well:

and then also a short paragraph to the details below, right at the beginning:

https://keda.sh/docs/2.10/concepts/scaling-jobs/#scaledjob-spec

Thanks!

[InbarRose]

i have the code and the docs.
I am not sure where to put tests. I do not see a place for scaled job tests, nor for labels.

[JorTurFer]

I am not sure where to put tests. I do not see a place for scaled job tests, nor for labels.

I answered you there :)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[InbarRose]

sorry. i am just not able to do the tests for this.
I do not have the time to implement a whole framework. would have been happy to add a single test to existing framework which simulates these.
so either someone else takes this to make tests, or this will just end here. 🤷

[JorTurFer]

Hi @InbarRose
I commented on the PR some time ago: #4600 (comment)
In any case, if you think that it requires a new test framework due to any reason, you could add an e2e test to check it directly on a real k8s cluster.
As it's something internal, you could add a test case here

In any case, I think that unit test is easier an faster. You are right with your point of having to create mocks for it, but you can extract the job generation logic to another function and cover it with unit tests.
Instead of doing a for and generating the jobs + creating it on the cluster, you could generate all the jobs first, and then create them on the cluster. With this change, you can add easily a unit test to cover the job spec.

Sorry for bothering you with the tests, but they are important to guarantee the behaviour in the future. If I build something using this new annotations, I'd like to have at least an small guarantee that they won't be removed by error. Other stuff could be covered out-of-the-box with already existing tests, but nothing is covering this specific scenario

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[josefkarasek]

Hey @zroubalik, I see that the only thing missing to resolve this issue is covering the new feature with tests.
I'll be happy to contribute them. Please assign the issue to me.