Repository navigation
fix(backups): CI was broken after merge - #2762
Conversation
Signed-off-by: Andrey Kolkov <[email protected]>
github-actions
Bot
added
size/M
📝 WalkthroughWalkthroughThis PR adds a new helper function ChangesJob strategy template rendering helper
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request restores missing template rendering logic for the job strategy controller that was lost during a previous rebase. By introducing a dedicated rendering entrypoint, the controller can now correctly process PodTemplateSpec configurations, ensuring consistency with other backup drivers in the system. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the renderJobTemplate function in internal/backupcontroller/jobstrategy_controller.go. This function templates a strategy's PodTemplateSpec using the repository's text/template engine, exposing a context map that includes Application, Release, Mode, Parameters, and optionally Backup details (such as ApplicationRef). This mirrors the template rendering patterns used by other drivers like Altinity. I have no feedback to provide as there are no review comments.
|
Actionable comments posted: 0 |
Andrey Kolkov (androndo)
changed the title
Timofei Larkin (lllamnyp)
left a comment
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
LGTM
Scope of the change
Branch: fix/ci-after-merge (PR #2762) targeting main.
Single-file fix to internal/backupcontroller/jobstrategy_controller.go:
- Adds an import of
github.com/cozystack/cozystack/internal/template. - Adds a new package-level function
renderJobTemplate(corev1.PodTemplateSpec, map[string]interface{}, releaseName, releaseNamespace, mode string, parameters map[string]string, *backupsv1alpha1.Backup) (*corev1.PodTemplateSpec, error).
No other files touched. The build is currently broken on main because two existing call sites at lines 186 and 470 reference renderJobTemplate, but the function with that name was renamed to renderAltinityTemplate in PR #2716 (commit f17ead3) and never re-introduced in the Job driver file. This PR restores the missing symbol by adding a per-driver renderer.
Correctness
- The new
renderJobTemplatebody is byte-identical torenderAltinityTemplateinaltinitystrategy_controller.go:108-148except for the function name and the inline comment that cross-references the Altinity rationale. Both functions take the same signature, build the samectxMap, optionally inject.Backup.ApplicationRef, and delegate totemplate.Template(&tmpl, ctxMap). The two call sites pass exactly the arguments this signature expects —strategy.Spec.Template(acorev1.PodTemplateSpec), an unstructured app map, release name/namespace, mode constant (jobStrategyModeBackup/jobStrategyModeRestore), resolved/round-tripped parameters, andnilorbackupfor the optional source-app context. - The
sourceAPIGroupnil-deref guard forbackup.Spec.ApplicationRef.APIGroupis preserved. - The pattern matches the comment's claim about per-driver render entrypoints:
renderCNPGTemplate,renderEtcdTemplate,renderFoundationDBTemplate,renderAltinityTemplateeach live in their own file. Caveat: the CNPG/Etcd/FoundationDB renderers take typed app objects (*postgresapp.Postgresetc.) and a different signature, so the "convention" is "each driver owns its render entrypoint," not "the signatures are the same." Job and Altinity happen to share a signature because both consumemap[string]interface{}andcorev1.PodTemplateSpec. That asymmetry isn't introduced by this PR — it's the existing shape — and the comment in the new function explicitly says "Mirrors renderAltinityTemplate." Not a blocker.
Build / test verification
go build ./...— clean.go vet ./internal/backupcontroller/...— clean.go test ./internal/backupcontroller/...—ok github.com/cozystack/cozystack/internal/backupcontroller 0.577s.
The existing tests in internal/backupcontroller/jobstrategy_controller_test.go (TestReconcileJob_CreatesBatchJob, TestReconcileJob_CompletesAndCreatesBackup, TestReconcileJob_FailsOnJobFailed, TestReconcileJob_FailsOnMissingStrategy, TestReconcileJobRestore_CreatesBatchJobInTargetNamespace, TestReconcileJobRestore_CompletesSucceeds, TestReconcileJob_FailsOnUnmappableKind, TestReconcileJobRestore_FailsOnUnmappableTargetKind) all exercise reconcileJob / reconcileJobRestore, which call renderJobTemplate. On broken main these tests fail to compile (and therefore fail to run) because renderJobTemplate is undefined. With this PR they compile and pass. In particular TestReconcileJob_CreatesBatchJob asserts --app=app-test --mode=backup --bucket=gen-bucket --replicas=3 in the rendered container args, which proves .Application (replicas=3 from the live app), .Parameters (bucketName=gen-bucket), .Release.Name, and .Mode all reach the template engine through this function. That is exactly the regression test for "renderJobTemplate exists and produces the documented context." No additional unit test is needed.
Cosmetic notes (not blockers)
- The
renderJobTemplatebody duplicatesrenderAltinityTemplatebyte-for-byte. A small follow-up could factor the shared rendering into arenderPodTemplateSpechelper that both Altinity and Job call into — neither file would lose anything: the per-driver entrypoint convention is satisfied by both files retaining their named wrapper. The duplication is acceptable in this PR because it is the minimum diff that unbreaksmain; the consolidation can be a separate refactor. Recommended to track this as a follow-up issue. - The PR description says "PR #2716 was merged with this break — main is currently broken." This is accurate.
Documentation
No documentation references renderJobTemplate directly. The Job strategy is documented at examples/backups/... and at the driver level; the doc strings inside the new function correctly cross-reference renderAltinityTemplate and the per-driver naming convention. No doc drift introduced.
2 participants
What this PR does
Fix CI break root cause: internal/backupcontroller/jobstrategy_controller.go:138, :422 calls renderJobTemplate which no longer exists — the rebased commit cefbaad renamed it to renderAltinityTemplate in altinitystrategy_controller.go (per the codebase's per-strategy convention: renderCNPGTemplate, renderEtcdTemplate, renderFoundationDBTemplate) but didn't add the corresponding renderJobTemplate to the Job strategy file. PR #2716 was merged with this break — main is currently broken.
Screenshots
Release note
Summary by CodeRabbit