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.

Sign up for GitHub

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

Jump to bottom

Using immutable labels as service selectors #1152

Merged
merged 5 commits into from
Oct 20, 2022
Merged

Using immutable labels as service selectors #1152

merged 5 commits into from
Oct 20, 2022

Conversation

angelokurtis
Copy link
Contributor

fixes #1107

@angelokurtis angelokurtis requested a review from a team October 9, 2022 20:49
@pavolloffay
Copy link
Member

CI failed

pavolloffay
pavolloffay reviewed Oct 11, 2022
View reviewed changes
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

Could you please check the selector lebels in the e2e test ? ###

https://github.com/open-telemetry/opentelemetry-operator/blob/main/tests/e2e/smoke-simplest/00-assert.yaml#L43 and for the headless as well

pkg/collector/reconcile/service.go Outdated
// whereas 'labels' refers to the service
selector := labels
selector := collector.SelectorLabels(params.Instance)
selector["app.kubernetes.io/name"] = naming.Service(params.Instance)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this label? there is already "app.kubernetes.io/instance": naming.Truncate("%s.%s", 63, instance.Namespace, instance.Name),

@pavolloffay pavolloffay mentioned this pull request Oct 11, 2022
Merged
@angelokurtis angelokurtis force-pushed the bugfix/image-update branch 2 times, most recently from 6e29f4f to a014c63 Compare October 15, 2022 14:11
@angelokurtis
Copy link
Contributor Author

Could you please check the selector lebels in the e2e test ?

We could add a check for all other selector labels, but I'm not sure if we could for app.kubernetes.io/instance because its value contains the namespace created by KUTTL, and it's known only during execution.

What do you think? Any suggestion?

@VineethReddy02
Copy link
Contributor

@angelokurtis the upgrade tests are failing, can you look into them?

@pavolloffay pavolloffay merged commit 873711c into open-telemetry:main Oct 20, 2022
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
@angelokurtis @VineethReddy02
Using immutable labels as service selectors (open-telemetry#1152)
8c5835e 
* Using immutable labels as service selectors

* Fixing expected service selectors

* Removing unnecessary selector

Co-authored-by: Vineeth Pothulapati <vineethpothulapati@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavolloffay pavolloffay pavolloffay approved these changes

@VineethReddy02 VineethReddy02 VineethReddy02 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error updating container image version
3 participants
@angelokurtis @pavolloffay @VineethReddy02