feat(surveyor): emit metadata.namespace on all rendered resources#1159
Open
yhrn wants to merge 2 commits into
Open
feat(surveyor): emit metadata.namespace on all rendered resources#1159yhrn wants to merge 2 commits into
yhrn wants to merge 2 commits into
Conversation
The surveyor chart never populated metadata.namespace in its rendered
output, relying on `helm install -n <ns>` to apply the namespace via the
Kubernetes API instead. This works for direct-install flows but breaks
any workflow that renders templates statically and applies them later
(`helm template -n <ns> | kubectl apply -f -`, kustomize+helm,
ArgoCD/Flux): the manifest carries no namespace, so resources land in
the kubectl client's current context (typically "default") instead of
the namespace passed to `helm template`.
Add a `surveyor.namespace` helper following the `jsc.namespace` pattern
used by the nack chart in this repo, and emit `namespace:` in every
template's metadata block (Service, ServiceAccount, Deployment,
ConfigMap, HPA, Ingress, ServiceMonitor). Defaults to
`.Release.Namespace`; can be overridden via `.Values.namespaceOverride`.
Behaviour change matrix (previous → new):
helm install -n my-ns : works → works (same)
helm template -n my-ns | kubectl apply -f - : silently lands in
kubectl's current ns → lands in my-ns
helm template -n my-ns | kubectl apply -n my-ns -f -
: works → works (same; -n is now redundant but harmless)
helm template | kubectl apply -n target-ns -f -
: works (no manifest ns, kubectl -n wins) → resources land in
"default" because manifest ns now wins (antipattern; users should
pass `-n` to `helm template`)
|
Claude finished @yhrn's task in 1m 6s —— View job PR Review: feat(surveyor): emit metadata.namespace on all rendered resources
Overall: the approach is correct and the implementation is sound. The One real issue
The nack chart explicitly adds
Fix: add Minor notes (no action required)
The breaking-change matrix in the PR description accurately describes the behavior shift and is a nice addition. |
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
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
The surveyor chart never populated
metadata.namespacein its rendered output, relying onhelm install -n <ns>to apply the namespace via the Kubernetes API instead. This works fine for direct-install flows but breaks any workflow that renders templates statically and applies them later (helm template -n <ns> | kubectl apply -f -, kustomize+helm, ArgoCD/Flux): the manifest carries no namespace field, so resources land in the kubectl client's current context (typicallydefault) instead of the namespace passed tohelm template.This PR adds a
surveyor.namespacehelper following the samejsc.namespacepattern used by the nack chart in this repo, and emitsnamespace:in every template's metadata block (Service, ServiceAccount, Deployment, ConfigMap, HPA, Ingress, ServiceMonitor). It defaults to.Release.Namespaceand can be overridden via.Values.namespaceOverride.This was discovered while migrating to Pulumi's
pulumi-kubernetesv4.30, which bundleskustomize/apiv0.21 — that release intentionally changed the namespace transformer so the kustomization-levelnamespace:directive no longer post-injectsmetadata.namespaceinto helm output. Charts that don't emit the field themselves end up namespace-less.Behaviour change matrix
helm install -n my-nshelm template -n my-ns | kubectl apply -f -my-nshelm template -n my-ns | kubectl apply -n my-ns -f --nflag is now redundant but harmless)helm template | kubectl apply -n target-ns -f --nwins)defaultbecause manifest ns now winsThe last row is the only true breaking change. It's an antipattern (the right form is
helm template -n target-ns), but worth calling out.