Repository navigation
fix(platform): make mongodb-application depend on its operator - #3845
Conversation
github-actions
Bot
added
size/M
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe default MongoDB application package now depends on ChangesMongoDB application dependency
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Merge Risk: ⚪ Minimal · up to The MongoDB application now waits for its operator before becoming available, preventing creation before the required CRD exists. The change is localized and covered by a regression test; no actionable merge-blocking risk remains after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
myasnikovdaniil NOT LGTM. The change itself is right, the only blocker is the attribution trailer on the commit.
Business context: a tenant can create a MongoDB before the Percona operator and its CRD exist, because the mongodb application never waited for the mongodb operator.
The edge is still missing on current main, and the branch merges into main without conflicts. It matches postgres, mariadb, kafka, redis and etcd, which all list their operator. Both packages come from the paas bundle under the same condition, and nothing depends on cozystack.mongodb-application. The operator itself depends only on networking, prometheus-operator-crds and cert-manager, so there is no cycle, and hack/select-install.sh --validate agrees. The new test runs under make unit-tests. It goes red when I remove the edge and green when I put it back.
The blocker: commit fcec3b0 carries Assisted-By: Claude <[email protected]>. docs/agents/contributing.md lists this exact line under Review Blockers, and the trailer value must be LLM. CI stayed green only because this base predates the Commit trailers job. Main's hack/check-commit-trailers.sh rejects the commit with "attribution trailer value must be exactly LLM", so it will fail after a rebase. Please reword it to Assisted-by: LLM.
While rewording, I'd also drop "found while reviewing it" from the commit body and "it was found while reviewing that one" from the PR body. The PR body becomes the merge commit message, and how the bug was found is not a reason for the change. Keeping #3817 as the precedent is fine.
E2E on this head failed only in kubernetes-previous and kubernetes-latest, and the mongodb suite passed. A dependsOn edge on the mongodb application doesn't reach the Kamaji control-plane suites, so I don't count that against the PR. The branch is 1289 commits behind main, so the rebase for the reword also gets you a fresh run on current code.
|
#4595 adds slim platform variants where every operator is opt-in through |
fcec3b0 to
48371f4
Compare
The mongodb chart renders kind: PerconaServerMongoDB from psmdb.percona.com/v1, but cozystack.mongodb-application depended only on networking and cozystack-engine. So mongodb-rd registers the ApplicationDefinition as soon as the engine is up, and a tenant can create a MongoDB before the operator exists; its HelmRelease then fails with no matches for kind "PerconaServerMongoDB". This is the same defect #3817 fixed for etcd. Every other operator-backed app here already carries the edge. It also closes a test-selection gap: without it cozystack.mongodb-operator reaches no runnable suite, so every change to the operator escalates to a run of all suites instead of selecting mongodb. select-install.sh --validate reports the graph still has no cycle. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
48371f4 to
b8bc0df
Compare
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM. The trailer is fixed. I rebased onto main and reworded the commit myself, and also dropped the suite count from the commit and the PR body: it was already stale, and any count there goes stale again.
Nit, not worth another round: the comment in mongodb-application.yaml lists postgres, mariadb, kafka, redis and etcd, but clickhouse, foundationdb, rabbitmq, opensearch and valkey carry the edge too. "Every other operator-backed app here" is true without the list. The etcd precedent has the same shape, so I'm leaving it.
2 participants
The mongodb chart renders
kind: PerconaServerMongoDBfrompsmdb.percona.com/v1, butcozystack.mongodb-applicationdepended only oncozystack.networkingandcozystack.cozystack-engine. Somongodb-rdregisters the ApplicationDefinition as soon as the engine is up, a tenant can create a MongoDB before the operator exists, and its HelmRelease then fails withno matches for kind "PerconaServerMongoDB".Every other operator-backed application here already carries the edge. This is the same defect #3817 fixes for etcd.
It also closes a test-selection gap. Without the edge
cozystack.mongodb-operatorreaches no runnable suite, so every change to the operator escalates to a run of all suites instead of selectingmongodb. Seesrc_to_suites()inhack/select-e2e.sh.Checks
packages/core/platform/tests/sources_mongodb_application_dependson_test.yamlpins the edge alongside the baseline ones, mirroring the etcd guard. Mutation checked: removing the edge turns it red, restoring it turns it green.hack/select-install.sh --validatereports the graph still has no cycle, and all platform helm unit tests pass.The exposure this adds
An administrator who lists
cozystack.mongodb-operatorinbundles.disabledPackageswhile keeping the application now leaves itDependenciesNotReady. That is the same exposure postgres, mariadb, kafka, redis and etcd already carry, withPackage.spec.ignoreDependenciesas the escape hatch.Release note
Summary by CodeRabbit
Bug Fixes
Tests
Closes #3850