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

Fix Cronjob name #2717

Merged
merged 11 commits into from
Apr 3, 2024
Merged

Conversation

janario
Copy link
Contributor

@janario janario commented Mar 6, 2024

Description:

Here we will change the service name resolver to give precedence for CronJob instead of Job since it would be the owner.

Also we start to collect the owner of Job if it has one, in some cases CronJob.

Link to tracking Issue(s):

Testing:

Documentation:

@janario janario requested a review from a team March 6, 2024 08:22
@janario
Copy link
Contributor Author

janario commented Mar 6, 2024

Marking this as a draft, I plan to get back on this and a few other issues soon after some internal migration.

@pavolloffay
Copy link
Member

Could you please create a changelog entry for this?

@@ -502,6 +503,26 @@ func (i *sdkInjector) addParentResourceLabels(ctx context.Context, uid bool, ns
if uid {
resources[semconv.K8SJobUIDKey] = string(owner.UID)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be really nice to add an E2E test for this situation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

e2e done, pls give it another review :-)

@janario janario force-pushed the fix/cronjob-service-name branch from 025a871 to d53ff48 Compare March 23, 2024 16:48
@janario janario changed the title Draft: Fix Cronjob name Fix Cronjob name Mar 26, 2024
@janario
Copy link
Contributor Author

janario commented Mar 26, 2024

Ready,

@pavolloffay @iblancasa can you pls give it a review? :-)

@janario janario force-pushed the fix/cronjob-service-name branch from c5bea75 to a5e601e Compare March 28, 2024 12:40
@@ -502,6 +503,26 @@ func (i *sdkInjector) addParentResourceLabels(ctx context.Context, uid bool, ns
if uid {
resources[semconv.K8SJobUIDKey] = string(owner.UID)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add a unit test, while we're at it? LGTM otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I'll take a look and get back to you soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, pls give it another review :-)

@janario janario force-pushed the fix/cronjob-service-name branch from a5e601e to e011d61 Compare April 2, 2024 20:36
janario added 8 commits April 2, 2024 22:37
Signed-off-by: Janario Oliveira <[email protected]>
Signed-off-by: Janario Oliveira <[email protected]>
Signed-off-by: Janario Oliveira <[email protected]>
Signed-off-by: Janario Oliveira <[email protected]>
Signed-off-by: Janario Oliveira <[email protected]>
Signed-off-by: Janario Oliveira <[email protected]>
Signed-off-by: Janario Oliveira <[email protected]>
Signed-off-by: Janario Oliveira <[email protected]>
@janario janario force-pushed the fix/cronjob-service-name branch from e011d61 to 4b3f56e Compare April 2, 2024 20:38
janario added 3 commits April 2, 2024 22:40
Signed-off-by: Janario Oliveira <[email protected]>
Signed-off-by: Janario Oliveira <[email protected]>
Signed-off-by: Janario Oliveira <[email protected]>
@pavolloffay pavolloffay merged commit 35d8891 into open-telemetry:main Apr 3, 2024
31 checks passed
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* Fix Cronjob name

Signed-off-by: Janario Oliveira <[email protected]>

* Mapped Job permissions

Signed-off-by: Janario Oliveira <[email protected]>

* Add changelog

Signed-off-by: Janario Oliveira <[email protected]>

* Add Job rbac permissions

Signed-off-by: Janario Oliveira <[email protected]>

* Add e2e test case

Signed-off-by: Janario Oliveira <[email protected]>

* Parent resources unit test

Signed-off-by: Janario Oliveira <[email protected]>

* ChooseServiceName unit test

Signed-off-by: Janario Oliveira <[email protected]>

* Generated manifest

Signed-off-by: Janario Oliveira <[email protected]>

* Rename e2e

Signed-off-by: Janario Oliveira <[email protected]>

* Fix e2e tests

Signed-off-by: Janario Oliveira <[email protected]>

* Fix lint

Signed-off-by: Janario Oliveira <[email protected]>

---------

Signed-off-by: Janario Oliveira <[email protected]>
@janario janario deleted the fix/cronjob-service-name branch May 9, 2024 09:18
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.

CronJob is generating the service_name as a Job
4 participants