Remove environment overrides (moved to gitops)#84
Conversation
leandrobmarinho
left a comment
leandrobmarinho
left a comment
There was a problem hiding this comment.
Review
Summary: Pure deletion of the four per-env Helm override files (overrides/{development,lab,production,staging}.yaml), which now live in gitops. No Go/chart-template/CR changes.
Verified: the diff is deletion-only (0 additions), no workflow/Makefile/appset in this repo references overrides/*.yaml, and values.yaml ships self-contained defaults (valkey.enabled: true, telemetry off), so the chart still installs cleanly. I cross-checked gitops #165: it adds all five env overrides for this component and repoints the appset to $gitops-values/..., so nothing is stranded.
Merge-order (as the PR body flags): gitops #165 must merge FIRST — the live appset reads these override files until #165 repoints it, so merging this before #165 would break the ArgoCD sync. No blocking issues given that ordering.
shingonoide
left a comment
shingonoide
left a comment
There was a problem hiding this comment.
Approving. Independently verified the four deleted override files land byte-identical in gitops #165 (git blob hashes match for development, lab, production, and staging), and #165 repoints the appset valueFiles to the gitops copies. The merge-order warning in the description is accurate and important: the live appset tracks HEAD of this repo and still reads overrides/.yaml, so gitops #165 must merge first. Chart installs cleanly from values.yaml defaults (helm template passes), and the deleted files contain no secrets, only in-cluster DNS names and a Secret name reference.
One small ask for the follow-up that cleans up values-production.yaml: please also sweep the now-stale references to the removed directory. AGENTS.md line 164 still documents deployment/openfilter-pipelines-controller/overrides/, and values.yaml comments at lines 139, 203, and 245 still point readers to overrides/.yaml in this repo.
Per-environment/client value overrides no longer belong in this open-source repo. They now live in the private gitops repo alongside the other components' overrides; the ArgoCD appset reads them from there. This repo keeps only the generic chart and its default values.yaml. Depends on gitops PR #165 (moves the overrides + repoints the appset). Merge that first.
lucasmundim
force-pushed
the
cpd-sim-move-overrides-to-gitops
branch
from
4785cc1 to
4b6eea0
Compare
lucasmundim
left a comment
lucasmundim
left a comment
There was a problem hiding this comment.
You're right, and sorry I didn't action this before it merged. Swept the stale references in a follow-up — #86: dropped the overrides/ bullet in AGENTS.md and repointed the values.yaml comments (gpuNodeSelector / valkey.enabled / externalValkey) at the gitops overrides path.
On values-production.yaml: I left it out of that PR because it's not a duplicate of the gitops overrides/production.yaml — it carries 38 keys not present there (affinity, podSecurityContext/securityContext, valkey architecture/auth/persistence/resources, replicaCount, image.pullPolicy). It's unreferenced by any appset/CI (so not deployed), but removing it would drop that config rather than move it — so it needs a migrate-to-gitops-or-confirm-dead decision. Flagged in #86.
Completes shingonoide's #84 cleanup ask. This standalone 'sample production values' file predates the ArgoCD/gitops overrides pattern and is referenced by nothing (no appset, CI, Makefile, or docs) — so it deploys nowhere. Verified every production-relevant key is already covered by the chart defaults + the gitops overrides/production.yaml (telemetry, resources, security contexts; prod uses external valkey so the bundled-valkey config is moot). The only key unique to this file — affinity.podAntiAffinity — is a no-op at the effective replicaCount of 1; if prod ever scales to 2+ replicas, add anti-affinity to the gitops override then. Also keeps Plainsight-internal prod config (otel-collector host, plainsightai image) out of this OSS repo.
* docs: point overrides references at the gitops repo (post-#84) Follow-up to shingonoide's review on #84: with deployment/.../overrides/ removed (moved to gitops), sweep the now-stale references so no one looks for the deleted directory: - AGENTS.md: drop the overrides/ bullet; note overrides live in the gitops repo. - values.yaml comments (gpuNodeSelector, valkey.enabled, externalValkey): point at the gitops overrides instead of overrides/<env>.yaml. Not touching values-production.yaml here — it carries config NOT present in the gitops production override (affinity, security contexts, valkey persistence/resources, ...), so its removal needs a migrate-or-confirm-dead decision, tracked separately. * chart: remove orphaned values-production.yaml Completes shingonoide's #84 cleanup ask. This standalone 'sample production values' file predates the ArgoCD/gitops overrides pattern and is referenced by nothing (no appset, CI, Makefile, or docs) — so it deploys nowhere. Verified every production-relevant key is already covered by the chart defaults + the gitops overrides/production.yaml (telemetry, resources, security contexts; prod uses external valkey so the bundled-valkey config is moot). The only key unique to this file — affinity.podAntiAffinity — is a no-op at the effective replicaCount of 1; if prod ever scales to 2+ replicas, add anti-affinity to the gitops override then. Also keeps Plainsight-internal prod config (otel-collector host, plainsightai image) out of this OSS repo.
3 participants
What
Removes
deployment/openfilter-pipelines-controller/overrides/(per-environment value overrides). The generic chart + defaultvalues.yamlstay.Why
This is an open-source repo; it shouldn't carry Plainsight-internal environment/client deployment config. Those overrides now live in the private
gitopsrepo alongside every other component's overrides (and theimage-tag.yamlthat was already there). The ArgoCD appset reads them from gitops.Depends on gitops PR #165, which moves these files into gitops and repoints the appset. Merge #165 first — otherwise the live appset still expects
overrides/<env>.yamlhere and the sync will fail.Note
deployment/.../values-production.yaml(a separate, non-appset-referenced file) is left as-is; cleaning that up can be a follow-up.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.