Skip to content

Common labels#2097

Open
vrutkovs wants to merge 11 commits intomasterfrom
commonLabels
Open

Common labels#2097
vrutkovs wants to merge 11 commits intomasterfrom
commonLabels

Conversation

@vrutkovs
Copy link
Copy Markdown
Collaborator

@vrutkovs vrutkovs commented Apr 23, 2026

This ensures HTTPRoutes and PVCs include ManagedMetadata labels and annotations.

Fixes #2094

TODO:

  • Implement passing commonLabel/managedMetadata in pod templates too

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/ptr"

"github.com/VictoriaMetrics/operator/internal/config"
Copy link
Copy Markdown
Contributor

@AndrewChubatiuk AndrewChubatiuk Apr 23, 2026

Choose a reason for hiding this comment

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

let's avoid adding any github.com/VictoriaMetrics/operator/internal/* package in api. probably it can be done in defaults.go instead

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 34 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/config/config.go">

<violation number="1" location="internal/config/config.go:630">
P2: Use the documented `envKeyValSeparator` tag; `envKeyValueSeparator` is ignored, so the new label/annotation env vars won't parse as `key=value` pairs.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.

Comment thread internal/config/config.go Outdated
Copy link
Copy Markdown
Contributor

@AndrewChubatiuk AndrewChubatiuk left a comment

Choose a reason for hiding this comment

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

Overall looks good! could you please also add test to cover an attempt to overwrite standard labels using commonLabels?

@vrutkovs vrutkovs force-pushed the commonLabels branch 2 times, most recently from bdb8758 to 07658b7 Compare April 23, 2026 16:46

scheme.AddTypeDefaultingFunc(&appsv1.DaemonSet{}, addDefaultMetadata)
scheme.AddTypeDefaultingFunc(&corev1.ConfigMap{}, addDefaultMetadata)
scheme.AddTypeDefaultingFunc(&corev1.Namespace{}, addDefaultMetadata)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not sure this is needed, we don't manage namespaces

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also don't understand why this function should be applied to all core k8s resources? isn't it enough to extend addDefaultMetadata to patch ManagedMetadata? also not sure if this should be applied to pods as well

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'd rather add those preemptively, as some feature may start managing those resources later.

also don't understand why this function should be applied to all core k8s resources?

My understanding is that if the object (VMAgent) creates a deployment and the service, we should make sure both have propagated labels and annotations. I'm not sure that omitting pods / child resources is sufficient for OPA.

also not sure if this should be applied to pods as well

I think so, yeah

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.

commonLabels and commonAnnotations

2 participants