Repository navigation
feat: commonAnnotations, Fusillade strategy and grace period, and API-only extra env and volumes - #112
feat: commonAnnotations, Fusillade strategy and grace period, and API-only extra env and volumes#112fergusfinn wants to merge 1 commit into
Conversation
…-only extra env and volumes Values that let a GitOps repository render the chart natively instead of patching its output: annotations on every object (an object's own key wins, and checksums ignore them, so setting them rolls no pods), the Fusillade daemons' rollout strategy and termination grace period, and environment variables, volumes and mounts for the API pods alone, rendered before the chart's own. All default to empty, so default renders are unchanged. Claude-Session: https://claude.ai/code/session_016Z9n119xkLAjqeNAWncB8R
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: 809e1d4687
ℹ️ 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".
| {{- with $root.Values.extraVolumes }} | ||
| {{- toYaml . | nindent 8 }} |
There was a problem hiding this comment.
When extraVolumes contains a chart-owned name such as config, model-provisioning, or email-templates, this emits both the user entry and the built-in entry. Kubernetes requires volume names within a PodSpec to be unique, so the Deployment is rejected rather than the later chart-owned volume “winning” as documented in values.yaml. Merge or filter these lists by name, or reject collisions explicitly.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 26 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="values.yaml">
<violation number="1" location="values.yaml:284">
P2: The precedence claim also covers volumes, but `extraVolumes` is concatenated with chart volumes, so a duplicate built-in name leaves a duplicate volume entry instead of making the chart entry win. Limit the precedence claim to `extraEnv` and state that extra volume names must be unique.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| # entries), volumes and volume mounts rendered before the chart's own, so the | ||
| # chart's own entries win on a duplicate name. |
There was a problem hiding this comment.
P2: The precedence claim also covers volumes, but extraVolumes is concatenated with chart volumes, so a duplicate built-in name leaves a duplicate volume entry instead of making the chart entry win. Limit the precedence claim to extraEnv and state that extra volume names must be unique.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At values.yaml, line 284:
<comment>The precedence claim also covers volumes, but `extraVolumes` is concatenated with chart volumes, so a duplicate built-in name leaves a duplicate volume entry instead of making the chart entry win. Limit the precedence claim to `extraEnv` and state that extra volume names must be unique.</comment>
<file context>
@@ -276,6 +280,15 @@ volumeMounts: []
# readOnly: true
+# API pods only (not Fusillade): environment variables (Kubernetes EnvVar
+# entries), volumes and volume mounts rendered before the chart's own, so the
+# chart's own entries win on a duplicate name.
+extraEnv: []
</file context>
| # entries), volumes and volume mounts rendered before the chart's own, so the | |
| # chart's own entries win on a duplicate name. | |
| # entries), volumes and volume mounts. Only `extraEnv` is precedence-ordered; | |
| # use unique volume names because volume entries are concatenated, not merged. |
Adds values that let internal render the control layer with native Argo CD Helm sources, instead of patching the chart's output with helmfile transformers. All of them default to empty, so default renders are unchanged. Nothing restarts until internal sets the new values.
New values
commonAnnotations: annotations added to every rendered object, hooks included. Where an object sets the same key itself (the migration hooks' sync waves,serviceAccount.annotations, the keystore StorageClass), the object's own value wins. Pod checksums are computed without them (control-layer.checksum), so setting or changing them rolls no pods.fusillade.strategy,fusillade.terminationGracePeriodSeconds: the Fusillade Deployments' rollout strategy and pod termination grace period, overridable per daemon underfusillade.split.<daemon>. When unset, nothing is rendered and Kubernetes' defaults apply.extraEnv,extraVolumes,extraVolumeMounts: env vars (as EnvVar entries), volumes and mounts for the API pods only; Fusillade doesn't inherit them. They render before the chart's own entries, so on a duplicate env name the chart's own value wins.Verified
helm templateonmainand on this branch is byte-identical for 8 value sets: defaults, ServiceMonitor, external database, split and single Fusillade, keystore with managed StorageClass and ServiceAccount annotations, the migration Job, and heap profiling with bootstrap, email templates and model provisioning.control-layerrelease (chart 1.10.0 values) against this chart, setting the new values instead of internal's transformers. Against today's patched render, all 19 objects match except the migration Job's two checksum annotations, which change because the hook config's wave is set explicitly. The Job is a PreSync hook recreated on every sync, so this rolls nothing. Internal'sinternal-git-shaprovenance annotation, which nothing reads, was excluded from the comparison.helm unittest .: 158 passed, including new cases intests/common_annotations_test.yaml,tests/fusillade_deployment_test.yamlandtests/control_layer_deployment_test.yaml.helm lintandjust testpass.https://claude.ai/code/session_016Z9n119xkLAjqeNAWncB8R