-
Notifications
You must be signed in to change notification settings - Fork 546
fix(labels): ensure app.kubernetes.io/version reflects image tag on workloads #4243
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
Open
andrefrco
wants to merge
6
commits into
open-telemetry:main
Choose a base branch
from
andrefrco:fix-app-kubernetes-io-version-label
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
fix(labels): ensure app.kubernetes.io/version reflects image tag on workloads #4243
andrefrco
wants to merge
6
commits into
open-telemetry:main
from
andrefrco:fix-app-kubernetes-io-version-label
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Fix version label logic in Collector, OpAMPBridge, and TargetAllocator manifests - Add fallback logic: spec image > config image > latest - Add comprehensive tests for all components (Collector, OpAMPBridge, TargetAllocator) - Fix expectations for default config values (0.0.0 instead of latest) Fixes open-telemetry#4175 Signed-off-by: andrefrco <[email protected]> fix(manifests): fix app.kubernetes.io/version label logic
I'll check the e2e tests failure asap |
iblancasa
reviewed
Aug 7, 2025
Replace len(image) == 0 with image == "" for better string validation in Collector, OpAMPBridge, and TargetAllocator deployment manifests.
- Add config parameter to updateCollectorStatus for consistent image resolution - Remove hardcoded version assertion from kubeletstats e2e test
Fixed the e2e test by removing the hardcoded version assertion from kubeletstats test (to avoid version-specific checks). Also added the same image resolution logic to UpdateCollectorStatus for consistency since it creates labelSelectors. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
This PR makes sure the
app.kubernetes.io/version
label on workloads (Deployment, DaemonSet, and StatefulSet) matches the tag from the image passed via the--collector-image
,--target-allocator-image
, or--operator-opamp-bridge-image
CLI flags in cases where.spec.image
isn’t set directly in the OpenTelemetryCollector, OpAMPBridge, or TargetAllocator CR.Before this, if
.spec.image
wasn’t defined, the operator would fall back to setting "latest" as the label even though the actual image (like 0.129.1) came from the CLI flag. This caused theservice.version
resource attribute to be incorrect in the workloads.Link to tracking Issue(s):
app.kubernetes.io/version
is wrong when using global--collector-image
option #4175Testing:
Added 3 unit tests per workload type:
Fixed a DefaultTests that incorrectly expected "latest" when using config.New(). Since config.New() returns a config with a fallback value (e.g.) used for tests, the test was updated to reflect that version "0.0.0" instead of "latest".
I've also tested the change locally using minikube and confirmed that the app.kubernetes.io/version label is now correctly set on the generated workloads.
Documentation:
This is a bug fix, no doc changes required.
Design consideration:
I thought about changing the
labels.Labels()
function to accept aconfig.Config
parameter, which would let us handle the version resolution in a single place. Something like:That way, we could reuse this logic across all components (services, serviceMonitors, etc) using the resourceImage and filterLabels together.
But I was a bit worried it might make the labels package more complex than it needs to be, since it would have to know about operator components and global config.
So for now, I went with a simpler approach and kept the logic outside. I'm new here and happy to improve this if there's a better way 🙂