fix(chart): honour namespaceOverride for the ingester Service - #587
Merged
Conversation
With namespaceOverride set, Service/diode-ingester was created in the release namespace while its Deployment landed in the override namespace, so the Service selected no pods and ingestion was unreachable. It was the only diode-owned template still using .Release.Namespace directly; the other 18, including the equivalent reconciler Service, already went through the diode.namespace helper. Also fix diode.auth.hostname, which built its FQDN from .Release.Namespace while Service/diode-auth follows the override, so the address pointed at a namespace the Service was not in. The four dependency hostnames (postgresql, redis, hydra public/admin) are deliberately left on .Release.Namespace, with a comment saying why: subcharts do not follow namespaceOverride, so their Services genuinely stay in the release namespace. Rendering shows diode-postgresql, diode-redis-* and diode-hydra-* there while diode's own resources move. Pointing those helpers at diode.namespace, as the original report suggested, would have broken working deployments. That also removes the need to promote namespaceOverride to a global value: the Hydra issuer in values.yaml targets a subchart Service, so its use of .Release.Namespace is correct as it stands. namespaceOverride was undocumented in both values.yaml and the README, which is most of why this went unnoticed. Documented, including the caveat that dependencies stay in the release namespace. Verified by rendering with `-n rel-ns` and `namespaceOverride: my-ns`: Service/diode-ingester moves rel-ns -> my-ns and matches its Deployment; diode-auth resolves to my-ns; postgres, redis and hydra still resolve to rel-ns. With default values, and with namespaceOverride equal to the release namespace, output is byte-identical to develop. Fixes MON-329 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
leoparente
requested review from
davidlanouette,
grant-nbl,
jajeffries,
manrodrigues,
mfiedorowicz,
paulstuart and
samiura
as code owners
August 31, 2026 19:37
samiura
approved these changes
Aug 31, 2026
Contributor
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bffb21e8fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
diode-postgresql-initdb-scripts-configmap and diode-hydra-extra-initcontainer-configmap were rendered into the override namespace, while the pods that consume them (StatefulSet/diode-postgresql, Deployment/diode-hydra, Job/diode-hydra-automigrate) stay in the release namespace because subcharts do not follow namespaceOverride. ConfigMap volume and envFrom references are namespace-scoped, so PostgreSQL could not mount its initdb script and Hydra's init container could not read its env. Same rule as the dependency hostname helpers: diode-owned resources that a subchart consumes belong alongside their consumer, not with diode's own workloads. Commented in both templates so the reason travels with the code. Pre-existing rather than introduced here, but this PR documents namespaceOverride as supported, so leaving it would have documented a configuration that cannot start. Verified: with namespaceOverride set, both ConfigMaps and all three consumers now render into the release namespace. With default values the rendered objects are semantically identical to develop (70 objects, zero differing); the only textual change is the added comments. Caught in review by Codex on #587. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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.
Fixes MON-329.
Problem
With
namespaceOverrideset,Service/diode-ingesteris created in the release namespace while its Deployment lands in the override namespace. The Service selects no pods and ingestion is unreachable.Rendering
-n rel-nswithnamespaceOverride: my-nson develop:diode-ingester-service.yaml:6was the only diode-owned template still using.Release.Namespacedirectly. The other 18, including the equivalent reconciler Service, already go throughdiode.namespace.diode.auth.hostnamehad the same mismatch in reverse: it built its FQDN from.Release.NamespacewhileService/diode-authdoes follow the override, so it addressed a namespace the Service wasn't in.The part the original report got wrong
MON-329 proposed changing all five hostname helpers to use
diode.namespace. That would have broken working deployments, and I only caught it by checking where the subchart Services actually land:Subcharts don't follow
namespaceOverride. Their Services genuinely stay in the release namespace, sodiode.postgresql.hostname,diode.redis.hostnameand the two hydra helpers pointing at.Release.Namespaceare correct today. Onlydiode.auth.hostnametargets a diode-owned Service, so only it changes. The four that stay now carry a comment explaining why, so they don't get "fixed" later.That also dissolves the third item in the ticket. The Hydra issuer in
values.yamltargets a subchart Service, so its.Release.Namespaceis right as it stands — no need to promotenamespaceOverrideto a global value, and no breaking values change.Documentation
namespaceOverrideappeared in neithervalues.yamlnor the README despitediode.namespacesupporting it, which is most of why this went unnoticed. Now documented, including the caveat that dependencies stay in the release namespace.Verification
With
-n rel-nsandnamespaceOverride: my-ns:Service/diode-ingesterdiode-authFQDNWith default values, byte-identical to develop once the chart version label is held constant. Same with
namespaceOverrideset equal to the release namespace.helm lintclean. Chart bumped1.15.2→1.15.3.🤖 Generated with Claude Code