Skip to content

namespace added to Helm Chart#345

Open
kevin-kho wants to merge 3 commits into
NVIDIA:mainfrom
kevin-kho:kh-chart-namespace
Open

namespace added to Helm Chart#345
kevin-kho wants to merge 3 commits into
NVIDIA:mainfrom
kevin-kho:kh-chart-namespace

Conversation

@kevin-kho

Copy link
Copy Markdown
Contributor

Description

Adds metadata.namespace to namespace scoped resources in the Helm Chart Templates.
Addresses #344

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • All commits are signed off per DCO (git commit -s).

kevin-kho added 3 commits June 8, 2026 08:59
Signed-off-by: Kevin Ho <kevinkh08@gmail.com>
Signed-off-by: Kevin Ho <kevinkh08@gmail.com>
Signed-off-by: Kevin Ho <kevinkh08@gmail.com>
@kevin-kho kevin-kho requested a review from dmitsh as a code owner June 8, 2026 16:14
@copy-pr-bot

copy-pr-bot Bot commented Jun 8, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@greptile-apps

greptile-apps Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds metadata.namespace to all namespace-scoped Helm chart resources (Deployments, DaemonSets, Services, ServiceAccounts, ConfigMaps, Ingress, and HTTPRoute) across the topograph chart and its two sub-charts, addressing issue #344 where resources could be placed in the wrong namespace.

  • Conditional guard is non-standard: Every template uses {{- if ne .Release.Namespace \"default\" }} to skip emitting the namespace for the default namespace. The Helm-recommended pattern is an unconditional namespace: {{ .Release.Namespace }}; omitting the field for default means GitOps tools can misplace resources when the cluster context's default namespace differs.
  • Test Pod templates overlooked: templates/tests/test-healthz.yaml and templates/tests/test-metrics.yaml are namespace-scoped Pod resources that were not updated, leaving them inconsistent with the rest of the fix.
  • RBAC and ServiceMonitor are intentionally unchanged: ClusterRole/ClusterRoleBinding are cluster-scoped (no namespace needed on metadata), and ServiceMonitor uses a dedicated .Values.serviceMonitor.namespace value — both are correctly left untouched.

Confidence Score: 3/5

The change correctly addresses namespace placement for non-default namespaces but leaves a gap for resources deployed to default and omits the test Pod templates entirely.

All 11 templates use a conditional that skips the namespace field when deploying to the default namespace. Rendered manifests from helm template or a GitOps pipeline have no explicit namespace for that case, and the cluster's contextual default — which may differ — will be used instead. Additionally, two namespace-scoped test Pod templates were not updated, keeping them inconsistent with the rest of the fix. Both gaps could reproduce the original misplacement bug in specific deployment patterns.

All 11 changed templates share the same conditional guard pattern; charts/topograph/templates/tests/test-healthz.yaml and test-metrics.yaml were not updated and still lack a namespace field.

Important Files Changed

Filename Overview
charts/topograph/templates/deployment.yaml Adds namespace conditionally, excluding the "default" namespace — non-standard pattern that leaves the default-namespace case without an explicit namespace field.
charts/topograph/templates/serviceaccount.yaml Same conditional ne "default" namespace pattern as deployment.yaml; service account will lack an explicit namespace when deploying to "default".
charts/topograph/templates/service.yaml Same conditional namespace guard; otherwise correct addition of namespace metadata.
charts/topograph/templates/configmap.yml Same conditional namespace guard on ConfigMap resource.
charts/topograph/templates/httproute.yaml Namespace conditionally added to HTTPRoute; same pattern issue as other templates.
charts/topograph/templates/ingress.yaml Namespace conditionally added to Ingress resource; same pattern issue.
charts/topograph/charts/node-data-broker/templates/daemonset.yaml DaemonSet gets conditional namespace; omits namespace field when deploying to "default".
charts/topograph/charts/node-data-broker/templates/serviceaccount.yaml ServiceAccount conditionally namespaced; same concern as other templates in this PR.
charts/topograph/charts/node-observer/templates/configmap.yml ConfigMap namespace added with same conditional guard.
charts/topograph/charts/node-observer/templates/deployment.yaml Deployment in node-observer sub-chart receives the same conditional namespace block.
charts/topograph/charts/node-observer/templates/serviceaccount.yaml ServiceAccount conditionally namespaced; consistent with rest of PR but shares the same pattern issue.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[helm install / helm template] --> B{Release.Namespace}
    B -->|"== 'default'"| C[Namespace field OMITTED from manifest]
    B -->|"!= 'default'"| D[namespace: Release.Namespace emitted]
    C --> E{Applied by?}
    E -->|"helm install --namespace default"| F[Helm API call sets namespace ✓]
    E -->|"kubectl apply / GitOps tool"| G[kubectl context default namespace used ⚠️]
    D --> H[Resource lands in correct namespace ✓]
    G --> I{Context namespace == default?}
    I -->|Yes| J[Correct namespace ✓]
    I -->|No| K[Wrong namespace — issue #344 still present ✗]
Loading

Reviews (1): Last reviewed commit: "added namespace to topograph node-data-b..." | Re-trigger Greptile

Comment on lines +8 to +10
{{- if ne .Release.Namespace "default" }}
namespace: {{ .Release.Namespace }}
{{- end}}

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.

P1 Non-standard conditional omits namespace for default

Every changed template guards the namespace field with {{- if ne .Release.Namespace "default" }}, which means rendering with --namespace default (or no --namespace flag) produces manifests without an explicit namespace: field. When those manifests are applied through a GitOps tool such as ArgoCD or Flux, the resource is placed into whatever the cluster's contextual default namespace is rather than default — recreating the very bug (#344) this PR intends to fix, just scoped to that one namespace.

The Helm-recommended pattern is to emit the namespace unconditionally: namespace: {{ .Release.Namespace }}. The same conditional appears in all 11 files changed by this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was to retain compatibility with how helm template functioned before.

If user specifies namespace=default or omits it, they are free to kubectl apply to any namespace they choose.

If namespace: default is in the manifest, it must be applied to the default namespace.

Can remove the conditional if we are fine with changing the default behavior.

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.

1 participant