Fix: Update related kind labels and allow overriding#754
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBumps multiple Go module versions and changes label handling: adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 403-414: The go.mod contains mismatched OpenTelemetry versions:
core modules (go.opentelemetry.io/otel, /trace, /metric, /sdk, etc.) are at
v1.42.0 while OTLP exporters
(go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp,
/otlpmetric/otlpmetrichttp, /otlptrace, /otlptrace/otlptracehttp) are still at
v1.38.0; update those exporter module versions to v1.42.0 to align with the core
modules (or alternatively downgrade the core modules to v1.38.0 if that is
required), ensuring go.mod entries for otlploghttp, otlpmetrichttp, otlptrace,
and otlptracehttp match the chosen version and then run go mod tidy to apply the
changes.
In `@pkg/objectcache/shared_container_data.go`:
- Around line 102-108: The code may produce empty normalized related labels
(e.g., labels[i] becomes "") after the switch on
helpersv1.RelatedKindMetadataKey / helpersv1.RelatedNameMetadataKey using
wlid.GetKindFromWlid or wlid.GetNameFromWlid; add a post-switch guard that
checks if labels[i] == "" and, if so, skip validation/processing for that entry
(same cleanup behavior as earlier) before calling content.IsLabelValue so you
don't emit incomplete metadata. Locate the switch block that assigns labels[i],
insert the empty-string check immediately after it, and return/continue/omit
that label from further validation/emit paths referenced by
content.IsLabelValue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a456c779-5c9b-483d-92e9-e0297b8a5a79
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.modpkg/objectcache/shared_container_data.go
matthyx
force-pushed
the
backend-integration
branch
from
c0e73f7 to
4eab851
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/objectcache/shared_container_data.go (1)
102-108:⚠️ Potential issue | 🟡 MinorAdd a post-normalization empty-value guard.
After Line 102-Line 105, a normalized related value can become empty (for example, kind
"ECS"→""), but empty cleanup currently happens only before the switch. Add a second empty check before Line 107 validation.💡 Proposed patch
switch i { case helpersv1.RelatedKindMetadataKey: labels[i] = strings.TrimPrefix(wlid.GetKindFromWlid(watchedContainer.Wlid), "ECS") case helpersv1.RelatedNameMetadataKey: labels[i] = wlid.GetNameFromWlid(watchedContainer.Wlid) } + if labels[i] == "" { + delete(labels, i) + continue + } errs := content.IsLabelValue(labels[i])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/objectcache/shared_container_data.go` around lines 102 - 108, After you normalize the related value inside the switch (the cases using wlid.GetKindFromWlid(watchedContainer.Wlid) and wlid.GetNameFromWlid(watchedContainer.Wlid) that assign to labels[i]), add a post-normalization guard that skips validation if labels[i] is now empty (e.g., if strings.TrimPrefix returned an empty string) before calling content.IsLabelValue(labels[i]); in short, ensure labels[i] is checked for == "" right after the switch and return/continue or skip adding errors so content.IsLabelValue is not run on an empty normalized value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/objectcache/shared_container_data.go`:
- Around line 102-108: After you normalize the related value inside the switch
(the cases using wlid.GetKindFromWlid(watchedContainer.Wlid) and
wlid.GetNameFromWlid(watchedContainer.Wlid) that assign to labels[i]), add a
post-normalization guard that skips validation if labels[i] is now empty (e.g.,
if strings.TrimPrefix returned an empty string) before calling
content.IsLabelValue(labels[i]); in short, ensure labels[i] is checked for == ""
right after the switch and return/continue or skip adding errors so
content.IsLabelValue is not run on an empty normalized value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4af0e9e-5088-4032-9e82-46cd79cc48ee
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.modpkg/objectcache/shared_container_data.go
matthyx
force-pushed
the
backend-integration
branch
from
4eab851 to
386e450
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 58: Update the pinned dependency for google.golang.org/grpc from v1.79.2
to v1.79.3 (or newer) in go.mod to address GHSA-p77j-4mvh-x3m3, then refresh
module metadata by re-resolving modules (e.g., run the equivalent of go get to
the new version and run go mod tidy) so go.sum is updated; target the dependency
named google.golang.org/grpc and ensure no other code references a stricter
version constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f03f7af7-9862-436e-9914-1cf08fc68a31
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.modpkg/objectcache/shared_container_data.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/objectcache/shared_container_data.go
matthyx
force-pushed
the
backend-integration
branch
from
386e450 to
e3beadf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
go.mod (1)
58-58:⚠️ Potential issue | 🔴 CriticalUpgrade vulnerable
google.golang.org/grpcbefore merge.Line 58 still pins
google.golang.org/grpc v1.79.2, which is affected by GHSA-p77j-4mvh-x3m3. Please bump tov1.79.3or newer and re-run module tidy.🔧 Proposed fix
- google.golang.org/grpc v1.79.2 + google.golang.org/grpc v1.79.3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 58, Update the pinned module google.golang.org/grpc from v1.79.2 to v1.79.3 (or newer) in the go.mod entry and then run `go mod tidy` to refresh go.sum and ensure dependencies are consistent; verify the updated version is committed so the GHSA-p77j-4mvh-x3m3 vulnerability is resolved.
🧹 Nitpick comments (1)
pkg/objectcache/shared_container_data_test.go (1)
133-140: Test case name is misleading.The name "no match case sensitivity" suggests the prefix shouldn't match, but the expected result
"o world"shows that"Hell"does match"Hello"case-insensitively. Consider renaming to something like"partial word prefix match"or"prefix matches start of word".✏️ Suggested name fix
{ - name: "no match case sensitivity", + name: "partial word prefix match", args: args{ s: "Hello world", prefix: "Hell", }, want: "o world", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/objectcache/shared_container_data_test.go` around lines 133 - 140, Rename the misleading test case name "no match case sensitivity" in the table-driven test in shared_container_data_test.go to accurately reflect the behavior (e.g., "prefix matches start of word" or "partial word prefix match"); update the name field of the specific test entry that has args {s: "Hello world", prefix: "Hell"} and want "o world" so the test name matches the expected outcome and clarifies intent for future readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/containerprofilemanager/v1/monitoring.go`:
- Around line 204-208: The Info log message contains a developer name and should
be removed or replaced; remove or rename the logger.L().Info call that logs
"Matthias container profile saved successfully" and instead emit a neutral
message (e.g., "container profile saved successfully") or drop the log entirely;
also stop discarding the error from watchedContainer.InstanceID.GetSlug(true) —
capture the returned error from GetSlug(true) and, if non-nil, log it at Debug
level (or handle appropriately) before using apName, referencing
watchedContainer.InstanceID.GetSlug, logger.L().Info, and
watchedContainer.InstanceID.GetStringFormatted to locate the code to change.
---
Duplicate comments:
In `@go.mod`:
- Line 58: Update the pinned module google.golang.org/grpc from v1.79.2 to
v1.79.3 (or newer) in the go.mod entry and then run `go mod tidy` to refresh
go.sum and ensure dependencies are consistent; verify the updated version is
committed so the GHSA-p77j-4mvh-x3m3 vulnerability is resolved.
---
Nitpick comments:
In `@pkg/objectcache/shared_container_data_test.go`:
- Around line 133-140: Rename the misleading test case name "no match case
sensitivity" in the table-driven test in shared_container_data_test.go to
accurately reflect the behavior (e.g., "prefix matches start of word" or
"partial word prefix match"); update the name field of the specific test entry
that has args {s: "Hello world", prefix: "Hell"} and want "o world" so the test
name matches the expected outcome and clarifies intent for future readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ae0caab-0090-4bd0-aec3-6c0ef676be74
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.modpkg/containerprofilemanager/v1/monitoring.gopkg/objectcache/shared_container_data.gopkg/objectcache/shared_container_data_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/objectcache/shared_container_data.go
Comment on lines
+204
to
+208
| apName, _ := watchedContainer.InstanceID.GetSlug(true) | ||
| logger.L().Info("Matthias container profile saved successfully", | ||
| helpers.String("apName", apName), helpers.String("name", slug), | ||
| helpers.String("namespace", container.K8s.Namespace), helpers.String("instanceID", watchedContainer.InstanceID.GetStringFormatted()), | ||
| helpers.String("wlid", watchedContainer.Wlid), helpers.Interface("annotations", containerProfile.ObjectMeta.Annotations), helpers.Interface("labels", containerProfile.ObjectMeta.Labels)) |
There was a problem hiding this comment.
Remove debug log with developer name before merging.
This appears to be a development/debug log that was accidentally included. The message "Matthias container profile saved successfully" contains a developer name and should not be committed to the codebase.
Additionally, the error from GetSlug(true) is silently discarded. If this logging is intended to remain (with a proper message), consider at least logging the error at Debug level if it occurs.
Suggested fix: Remove the debug log
logger.L().Debug("container profile saved successfully",
helpers.String("containerID", watchedContainer.ContainerID),
helpers.String("containerName", container.Runtime.ContainerName),
helpers.String("podName", container.K8s.PodName))
- apName, _ := watchedContainer.InstanceID.GetSlug(true)
- logger.L().Info("Matthias container profile saved successfully",
- helpers.String("apName", apName), helpers.String("name", slug),
- helpers.String("namespace", container.K8s.Namespace), helpers.String("instanceID", watchedContainer.InstanceID.GetStringFormatted()),
- helpers.String("wlid", watchedContainer.Wlid), helpers.Interface("annotations", containerProfile.ObjectMeta.Annotations), helpers.Interface("labels", containerProfile.ObjectMeta.Labels))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| apName, _ := watchedContainer.InstanceID.GetSlug(true) | |
| logger.L().Info("Matthias container profile saved successfully", | |
| helpers.String("apName", apName), helpers.String("name", slug), | |
| helpers.String("namespace", container.K8s.Namespace), helpers.String("instanceID", watchedContainer.InstanceID.GetStringFormatted()), | |
| helpers.String("wlid", watchedContainer.Wlid), helpers.Interface("annotations", containerProfile.ObjectMeta.Annotations), helpers.Interface("labels", containerProfile.ObjectMeta.Labels)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/containerprofilemanager/v1/monitoring.go` around lines 204 - 208, The
Info log message contains a developer name and should be removed or replaced;
remove or rename the logger.L().Info call that logs "Matthias container profile
saved successfully" and instead emit a neutral message (e.g., "container profile
saved successfully") or drop the log entirely; also stop discarding the error
from watchedContainer.InstanceID.GetSlug(true) — capture the returned error from
GetSlug(true) and, if non-nil, log it at Debug level (or handle appropriately)
before using apName, referencing watchedContainer.InstanceID.GetSlug,
logger.L().Info, and watchedContainer.InstanceID.GetStringFormatted to locate
the code to change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/objectcache/shared_container_data.go`:
- Around line 108-115: The override loop for watchedContainer.LabelOverrides
applies values without validation; update it to validate each non-empty override
with content.IsLabelValue before assigning to labels (keep the current behavior
of deleting when v == ""), and if content.IsLabelValue(v) returns false then
skip/delete that key (and optionally log a warning) instead of assigning the
invalid value; modify the loop over watchedContainer.LabelOverrides (the section
that assigns to the labels map) to perform this validation check before setting
labels[k] = v.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f204d68-00ee-47ce-85ee-ff292777b057
📒 Files selected for processing (3)
pkg/networkstream/v1/network_stream.gopkg/objectcache/shared_container_data.gopkg/objectcache/shared_container_data_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/objectcache/shared_container_data_test.go
matthyx
force-pushed
the
backend-integration
branch
from
17eb69d to
fd04838
Compare
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
backend-integration
branch
from
fd04838 to
3ab3a1d
Compare
matthyx
changed the title
…ntity mapping Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
jnathangreeg
approved these changes
Mar 27, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary by CodeRabbit