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

Job annotations #4600

Closed
wants to merge 1 commit into from
Closed

Job annotations #4600

wants to merge 1 commit into from

Conversation

InbarRose
Copy link

  • Update scale_jobs.go
  • Update CHANGELOG.md

add annotations from scaledjob spec to job spec
add new improvement to changelog

Relates to #4594

* Update scale_jobs.go
* Update CHANGELOG.md

add annotations from scaledjob spec to job spec
add new improvement to changelog

Signed-off-by: Inbar <5904674+InbarRose@users.noreply.github.com>
@InbarRose InbarRose requested a review from a team as a code owner June 1, 2023 09:15
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good! Could you add a unit test to validate the annotations?
Please, update docs as well 🙏

@InbarRose
Copy link
Author

looking good! Could you add a unit test to validate the annotations? Please, update docs as well pray

would be happy to add tests.
Can you help me understand where I should add them?

@JorTurFer
Copy link
Member

Can you help me understand where I should add them?

You can use this test as example: https://github.com/kedacore/keda/blob/main/pkg/scaling/executor/scale_jobs_test.go#L202-L214
I'd add a test case calling to scaleExecutor.createJobs and checking the generated job

@InbarRose
Copy link
Author

InbarRose commented Jun 1, 2023

Can you help me understand where I should add them?

You can use this test as example: https://github.com/kedacore/keda/blob/main/pkg/scaling/executor/scale_jobs_test.go#L202-L214 I'd add a test case calling to scaleExecutor.createJobs and checking the generated job

those are all using mocks to simulate the scaling decision.
I am happy to add a new test to an existing framework, but I don't think this is the correct scope to generate a whole new test framework.
If there is already an existing test framework which will generate a new job from a scaled job, please share where i can find it.

@JorTurFer
Copy link
Member

JorTurFer commented Jun 1, 2023

I am happy to add a new test to an existing framework, but I don't think this is the correct scope to generate a whole new test framework.

You don't need to generate the whole tests framework. There are other tests (I didn't link the correct one, sorry :( ) that mock the client to test only the executor: https://github.com/kedacore/keda/blob/main/pkg/scaling/executor/scale_jobs_test.go#L216-L287

You could do something similar, calling the client mock with an empty slice, implementing the Create function mock which just adds the new job to the slice, and checking the slice elements at the end of the call:

  1. Implement the mock function for client.Create, storing the new created jobs in the slice that the mock receives during the creation
  2. Add a test which starts the mock with an empty array and generate the scalerExecutor.
  3. Call to scaleExecutor.createJobs with the needed info
  4. After the end of the call, check the slice and evaluate if the job/jobs contain the annotations that you added in the ScaledJob

With this, you could check your code and you don't need to create the whole test framework, just adding the required mock functions is enough IMO

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @JorTurFer, adding a test would be great.

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@InbarRose any update on this please? We are about to do a new release.

@InbarRose
Copy link
Author

I haven't had time to add the tests.
But i see someone added the doc changes already
kedacore/keda-docs#1145

@zroubalik
Copy link
Member

Implemented in: #5106

@zroubalik zroubalik closed this Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants