fix(PL-6888): generate different catalog apps per service#25
Conversation
📝 WalkthroughWalkthroughThe chart now exposes the Helm release name as ChangesService name propagation
Build context and pull logging
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Helm
participant Deployment
participant Operator
participant CatalogReconciler
participant ArgoCD
Helm->>Deployment: render SERVICE_NAME from release name
Deployment->>Operator: provide SERVICE_NAME
Operator->>CatalogReconciler: pass cfg.ServiceName
CatalogReconciler->>ArgoCD: apply prefixed catalog Application
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/operator/main_test.go`:
- Around line 634-638: Update the polling interval passed to EventuallyNoErrorf
in the appsIntf.Get test from 50 seconds to a millisecond-scale duration,
keeping it shorter than the existing 2-second timeout so retries occur before
the test expires.
In `@cmd/operator/reconciler_catalog.go`:
- Line 53: Use a single prefixed Application name consistently across the apply
and delete branches in the reconciler, reusing the naming construction from the
apply path near Name and updating the delete logic accordingly. Add an upgrade
cleanup or migration path for existing unprefixed applications so disabling pull
mode removes both naming variants when applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a3e5007-4278-494f-8488-0380ccc11726
📒 Files selected for processing (9)
.dockerignorechart/templates/deployment.yamlchart/tests/common-labels/expected.yamlchart/tests/standard/expected.yamlcmd/operator/conf.gocmd/operator/main.gocmd/operator/main_test.gocmd/operator/puller.gocmd/operator/reconciler_catalog.go
| _, err := appsIntf.Get(t.Context(), "joy-operator-tests-catalog", metav1.GetOptions{}) | ||
| return err | ||
| }, | ||
| 50*time.Second, | ||
| 2*time.Second, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use a millisecond polling interval.
The test polls every 50*time.Second but times out after 2*time.Second. After an initial transient failure, EventuallyNoErrorf waits 50 seconds before retrying, so the test cannot honor its timeout and can stall CI.
Proposed fix
- 50*time.Second,
+ 50*time.Millisecond,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, err := appsIntf.Get(t.Context(), "joy-operator-tests-catalog", metav1.GetOptions{}) | |
| return err | |
| }, | |
| 50*time.Second, | |
| 2*time.Second, | |
| _, err := appsIntf.Get(t.Context(), "joy-operator-tests-catalog", metav1.GetOptions{}) | |
| return err | |
| }, | |
| 50*time.Millisecond, | |
| 2*time.Second, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/operator/main_test.go` around lines 634 - 638, Update the polling
interval passed to EventuallyNoErrorf in the appsIntf.Get test from 50 seconds
to a millisecond-scale duration, keeping it shorter than the existing 2-second
timeout so retries occur before the test expires.
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: event.Name, | ||
| Name: params.ServiceName + "-" + event.Name, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Use one Application name for both apply and delete.
Line 53 creates {SERVICE_NAME}-{catalog} applications, but the delete branch at Lines 81-83 still deletes only event.Name. If pull mode is disabled after an application was created, the prefixed Application remains and may continue syncing. Existing unprefixed applications also need an explicit migration or cleanup path during upgrade.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/operator/reconciler_catalog.go` at line 53, Use a single prefixed
Application name consistently across the apply and delete branches in the
reconciler, reusing the naming construction from the apply path near Name and
updating the delete logic accordingly. Add an upgrade cleanup or migration path
for existing unprefixed applications so disabling pull mode removes both naming
variants when applicable.
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: event.Name, | ||
| Name: params.ServiceName + "-" + event.Name, |
There was a problem hiding this comment.
Could you please elaborate why? 🙏
There was a problem hiding this comment.
Because otherwise we cannot have two different deployments generating different app-of-apps for the catalog (watching different envs)
There was a problem hiding this comment.
OK, but wouldn't that break the other reconcilers that expect catalog as the Catalog name?
davidmdm
changed the title
2 participants
Summary by CodeRabbit