eso: split cluster configs into separate overlays in staging - #13537
eso: split cluster configs into separate overlays in staging#13537sadlerap wants to merge 1 commit into
Conversation
Split each staging cluster's configuration into a separate overlay, which allows us to modify individual configurations for each cluster. While we're at it, move the IBM-specific ESO changes out that were requested in redhat-appstudio#13424. Fixes: KFLUXINFRA-4424 Signed-off-by: Andy Sadler <ansadler@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sadlerap The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Kustomize Render DiffComparing No render differences detected. |
|
Not entirely certain this is the right way to go about this, since we're going to need to reconsider how this works once ring deployments comes for ESO. Would like some feedback here. |
|
🤖 Finished Review · ✅ Success · Started 8:42 PM UTC · Completed 8:58 PM UTC Commit: |
PR Summary by QodoSplit ESO staging config into per-cluster overlays and isolate IBM CIDR exceptions
AI Description
Diagram
High-Level Assessment
Files changed (61)
|
Code Review by Qodo
1. ClusterDir override not applied
|
![qodo-for-redhat-appstudio[bot]](https://avatars.githubusercontent.com/u/89927105?s=60&v=4){kind=small}
| - nameNormalized: lightwell-dev | ||
| clusterDir: lightwell-dev | ||
| - nameNormalized: stone-stage-p01 | ||
| clusterDir: stone-stage-p01 |
There was a problem hiding this comment.
1. Clusterdir override not applied 🐞 Bug ≡ Correctness
In the external-secrets-operator ApplicationSet, the list generator sets clusterDir at the top
level, but the template renders the source path using {{values.clusterDir}}, leaving it empty.
This will make ArgoCD try to render from components/external-secrets-operator/staging/ instead of
the per-cluster overlay directories, causing manifest generation/sync to fail for the listed
clusters.
Agent Prompt
### Issue description
`external-secrets-operator.yaml` uses `{{values.clusterDir}}` in `spec.template.spec.source.path`, but the newly-added list elements set `clusterDir` (not `values.clusterDir`). As a result, `values.clusterDir` stays empty and the ApplicationSet will point ArgoCD at a directory that doesn’t contain the intended overlay kustomizations.
### Issue Context
This repo’s established pattern for per-cluster overrides in merge generators is to set dotted keys like `values.clusterDir: <dir>` in list elements.
### Fix Focus Areas
- argo-cd-apps/base/all-clusters/infra-deployments/external-secrets-operator/external-secrets-operator.yaml[12-32]
- argo-cd-apps/base/all-clusters/infra-deployments/cost-management/costmanagement-metrics-operator.yaml[16-24]
- components/external-secrets-operator/staging/base/kustomization.yaml[1-10]
- components/external-secrets-operator/staging/stone-stage-p01/kustomization.yaml[1-4]
### Concrete fix
Update the list elements to set `values.clusterDir` (repo-standard), e.g.
```yaml
- nameNormalized: lightwell-dev
values.clusterDir: lightwell-dev
```
(and similarly for the other clusters). Optionally, set a safe default for `values.clusterDir` in the clusters generator if you want non-overridden clusters to render a valid base.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ReviewFindingsCritical
High
Medium
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| - list: | ||
| elements: [] | ||
| elements: | ||
| - nameNormalized: lightwell-dev |
There was a problem hiding this comment.
[critical] api-contract
The list generator elements use bare clusterDir but the path template references {{values.clusterDir}}. In ArgoCD's merge generator, the clusters generator produces parameters in the values.* namespace. To override those in a list generator, the key must be values.clusterDir. Using bare clusterDir creates a separate top-level parameter that does not override values.clusterDir, so the template still reads the default empty string for all three clusters. Every other ApplicationSet in this repository uses values.clusterDir in list elements.
Suggested fix: Change the list element keys from clusterDir to values.clusterDir.
| @@ -15,7 +15,13 @@ spec: | |||
| environment: staging | |||
| clusterDir: "" | |||
There was a problem hiding this comment.
[high] logic-error
The default clusterDir value remains an empty string but all files have been moved from staging/ to staging/base/. Any cluster not explicitly listed would resolve to components/external-secrets-operator/staging/, which no longer contains a kustomization.yaml. The established pattern across the repo is to set clusterDir: base when introducing a base subdirectory.
Suggested fix: Change the default clusterDir value from an empty string to base.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13537 +/- ##
=======================================
Coverage 58.78% 58.78%
=======================================
Files 24 24
Lines 1548 1548
=======================================
Hits 910 910
Misses 559 559
Partials 79 79
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Risk Assessment
Risk Level: Medium |
|
@sadlerap the staging PR for this change has been soaking for 7d — but this production PR still has no reviewer approvals. Please request a review from a team member using the Reviewers panel on the right, or ask in your team's Slack channel. |
|
@sadlerap the staging PR for this change has been soaking for 2w — but this production PR still has no reviewer approvals. Please request a review from a team member using the Reviewers panel on the right, or ask in your team's Slack channel. |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
2 participants
Split each staging cluster's configuration into a separate overlay, which allows us to modify individual configurations for each cluster. While we're at it, move the IBM-specific ESO changes out that were requested in #13424.
Fixes: KFLUXINFRA-4424