feat(api): expose HelmRelease generation knobs as cozystack-api flags - #2571
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes HelmRelease timing and retry defaults into config.ReleaseConfig, adds ParsePositiveDuration utility for duration validation, exposes and parses server CLI flags, applies parsed defaults into ResourceConfig, updates HelmRelease spec generation to use config-driven values with per-application timeout overrides, and adds comprehensive tests throughout. ChangesHelmRelease Defaults Centralization
Sequence DiagramsequenceDiagram
participant CLI as CLI Flags
participant Server as Server Start
participant K8s as Kubernetes API
participant Config as ResourceConfig
participant REST as REST Layer
participant HR as HelmRelease Spec
CLI->>Server: provide helmrelease-* flag values
Server->>Server: parseAndValidateHelmReleaseFlags()<br/>via ParsePositiveDuration
activate Server
Server->>K8s: list ApplicationDefinition CRs
K8s-->>Server: return CR list
Server->>Config: apply server-wide HelmRelease defaults<br/>into each ResourceConfig entry
Config-->>Server: ResourceConfig populated
deactivate Server
Server->>REST: initialize REST with ResourceConfig
REST->>REST: for each Application, compute timeouts<br/>with per-app override precedence
REST->>HR: set Interval, MaxHistory, RetryOnFailure<br/>strategy with RetryInterval
REST->>HR: set Install/Upgrade Timeout from<br/>computed values, nil Remediation
HR-->>REST: HelmRelease spec ready
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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 significantly enhances the 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
|
There was a problem hiding this comment.
Code Review
This pull request refactors duration parsing into a shared utility and introduces global configuration flags for HelmRelease generation—including intervals, timeouts, and history limits—to ensure parity between the operator and API server. The conversion logic was updated to use these new defaults and transition to a structured retry strategy. Feedback identifies an opportunity to reduce boilerplate in the API server's flag validation by using a local helper function for duration parsing.
| hrInterval, err := config.ParsePositiveDuration("--helmrelease-interval", o.HelmReleaseInterval) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| hrRetryInterval, err := config.ParsePositiveDuration("--helmrelease-retry-interval", o.HelmReleaseRetryInterval) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| hrInstallTimeout, err := config.ParsePositiveDuration("--helmrelease-install-timeout", o.HelmReleaseInstallTimeout) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| hrUpgradeTimeout, err := config.ParsePositiveDuration("--helmrelease-upgrade-timeout", o.HelmReleaseUpgradeTimeout) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The repeated pattern for parsing duration flags could be simplified by using a small helper function within Complete. This would improve readability and reduce boilerplate, similar to how it's handled in the operator's main.go.
| hrInterval, err := config.ParsePositiveDuration("--helmrelease-interval", o.HelmReleaseInterval) | |
| if err != nil { | |
| return err | |
| } | |
| hrRetryInterval, err := config.ParsePositiveDuration("--helmrelease-retry-interval", o.HelmReleaseRetryInterval) | |
| if err != nil { | |
| return err | |
| } | |
| hrInstallTimeout, err := config.ParsePositiveDuration("--helmrelease-install-timeout", o.HelmReleaseInstallTimeout) | |
| if err != nil { | |
| return err | |
| } | |
| hrUpgradeTimeout, err := config.ParsePositiveDuration("--helmrelease-upgrade-timeout", o.HelmReleaseUpgradeTimeout) | |
| if err != nil { | |
| return err | |
| } | |
| parse := func(name, val string) (time.Duration, error) { | |
| return config.ParsePositiveDuration(name, val) | |
| } | |
| hrInterval, err := parse("--helmrelease-interval", o.HelmReleaseInterval) | |
| if err != nil { | |
| return err | |
| } | |
| hrRetryInterval, err := parse("--helmrelease-retry-interval", o.HelmReleaseRetryInterval) | |
| if err != nil { | |
| return err | |
| } | |
| hrInstallTimeout, err := parse("--helmrelease-install-timeout", o.HelmReleaseInstallTimeout) | |
| if err != nil { | |
| return err | |
| } | |
| hrUpgradeTimeout, err := parse("--helmrelease-upgrade-timeout", o.HelmReleaseUpgradeTimeout) | |
| if err != nil { | |
| return err | |
| } |
There was a problem hiding this comment.
This is addressed: the duration flags are parsed in a single helper, parseAndValidateHelmReleaseFlags() (pkg/cmd/server/start.go), which Complete() calls before any Kubernetes I/O. The repeated inline ParsePositiveDuration error-check blocks were folded into that helper, so Complete() no longer carries the boilerplate.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/config/config.go (1)
109-131: 💤 Low valueMinor:
omitemptyonHelmReleaseMaxHistorycollapses "unlimited" and "unset".
intwithomitemptydrops zero values on YAML marshal, so aHelmReleaseMaxHistory: 0(which Helm semantics treat as "unlimited") becomes indistinguishable from "field not present" ifReleaseConfigis ever serialized. Todaypkg/cmd/server/start.goalways populates this field from the--helmrelease-max-historyflag, so the runtime path is safe — but ifResourceConfigever gets persisted/round-tripped (telemetry dump, debug endpoint, future YAML config file), the explicit "unlimited" intent is lost.If you want to be future-proof, use a pointer (
*int) or dropomitemptyforHelmReleaseMaxHistoryspecifically. Not worth blocking on for this PR.🤖 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 `@pkg/config/config.go` around lines 109 - 131, HelmReleaseMaxHistory is declared as int with `yaml:"helmReleaseMaxHistory,omitempty"`, so a value of 0 (meaning "unlimited") is dropped on marshal and becomes indistinguishable from unset; change the field to either a pointer (*int) or remove `omitempty` on the HelmReleaseMaxHistory struct tag so zero is preserved when serialized, e.g., update the HelmReleaseMaxHistory declaration in the same struct where HelmReleaseInterval/HelmReleaseRetryInterval are defined and ensure any code that sets or reads HelmReleaseMaxHistory (e.g., server startup code that reads the --helmrelease-max-history flag) handles the pointer or non-omitempty semantics accordingly.pkg/config/config_test.go (1)
125-158: 💤 Low valueTest coverage matches the invariants the function promises.
Valid units, zero, negative, malformed, and empty inputs are all exercised, which is what the operator/api startup paths actually rely on.
Optional follow-up: the table doesn't differentiate the two error branches (
time.ParseDurationfailure vs.d <= 0). If you want to pin the error wording (e.g. that operators seemust be > 0for0s/-5mandinvalid durationfor5x/empty), add anerrMatchfield likeTestParseHelmInstallTimeoutAnnotationalready does — purely additive and keeps the two parsers' diagnostics from drifting.🤖 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 `@pkg/config/config_test.go` around lines 125 - 158, The tests for ParsePositiveDuration exercise the right cases but don't assert which error branch triggered; add an errMatch (or similar) field to the test table and in each case assert the returned error message matches the expected substring for that input (e.g., "must be > 0" for zero/negative cases and "invalid duration" or the ParseDuration error text for malformed/empty cases) so the test verifies both parse-failure vs non-positive-duration branches of ParsePositiveDuration.pkg/registry/apps/application/rest.go (1)
1545-1572: 💤 Low valueConsider adding a separate
release.cozystack.io/helm-upgrade-timeoutannotation for future flexibility.The current behavior where
release.cozystack.io/helm-install-timeoutsets both Install.Timeout and Upgrade.Timeout to the same value is intentional and documented (lines 1553-1554). Onlykubernetes-rdcurrently uses this annotation; the concern about operator deployments relying on asymmetric global timeouts (--helmrelease-install-timeoutand--helmrelease-upgrade-timeout) while also setting this annotation does not appear to apply to any existing deployment. That said, as noted in the code comment, the annotation mechanism is generic. To allow future ApplicationDefinitions to independently control upgrade timeout, adding a parallelrelease.cozystack.io/helm-upgrade-timeoutannotation would be a clean follow-up without changing current behavior.🤖 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 `@pkg/registry/apps/application/rest.go` around lines 1545 - 1572, The code currently treats release.cozystack.io/helm-install-timeout as controlling both install and upgrade timeouts; add support for a separate release.cozystack.io/helm-upgrade-timeout annotation so callers can override Upgrade.Timeout independently: when building helmRelease, read the new annotation (same parsing pattern used for HelmInstallTimeout), map it to a new variable (e.g., r.releaseConfig.HelmUpgradeTimeout or a local parsed value), and if present and >0 set upgradeTimeout from that value instead of copying HelmInstallTimeout; ensure you update the logic around installTimeout/upgradeTimeout and the assignments to helmRelease.Spec.Install.Timeout and helmRelease.Spec.Upgrade.Timeout to prefer the new upgrade-specific value while preserving existing behavior when the new annotation is absent.
🤖 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.
Nitpick comments:
In `@pkg/config/config_test.go`:
- Around line 125-158: The tests for ParsePositiveDuration exercise the right
cases but don't assert which error branch triggered; add an errMatch (or
similar) field to the test table and in each case assert the returned error
message matches the expected substring for that input (e.g., "must be > 0" for
zero/negative cases and "invalid duration" or the ParseDuration error text for
malformed/empty cases) so the test verifies both parse-failure vs
non-positive-duration branches of ParsePositiveDuration.
In `@pkg/config/config.go`:
- Around line 109-131: HelmReleaseMaxHistory is declared as int with
`yaml:"helmReleaseMaxHistory,omitempty"`, so a value of 0 (meaning "unlimited")
is dropped on marshal and becomes indistinguishable from unset; change the field
to either a pointer (*int) or remove `omitempty` on the HelmReleaseMaxHistory
struct tag so zero is preserved when serialized, e.g., update the
HelmReleaseMaxHistory declaration in the same struct where
HelmReleaseInterval/HelmReleaseRetryInterval are defined and ensure any code
that sets or reads HelmReleaseMaxHistory (e.g., server startup code that reads
the --helmrelease-max-history flag) handles the pointer or non-omitempty
semantics accordingly.
In `@pkg/registry/apps/application/rest.go`:
- Around line 1545-1572: The code currently treats
release.cozystack.io/helm-install-timeout as controlling both install and
upgrade timeouts; add support for a separate
release.cozystack.io/helm-upgrade-timeout annotation so callers can override
Upgrade.Timeout independently: when building helmRelease, read the new
annotation (same parsing pattern used for HelmInstallTimeout), map it to a new
variable (e.g., r.releaseConfig.HelmUpgradeTimeout or a local parsed value), and
if present and >0 set upgradeTimeout from that value instead of copying
HelmInstallTimeout; ensure you update the logic around
installTimeout/upgradeTimeout and the assignments to
helmRelease.Spec.Install.Timeout and helmRelease.Spec.Upgrade.Timeout to prefer
the new upgrade-specific value while preserving existing behavior when the new
annotation is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e7d848df-72cf-42ec-b358-550ad0c64643
📒 Files selected for processing (8)
cmd/cozystack-operator/main.gocmd/cozystack-operator/main_test.gopkg/cmd/server/start.gopkg/config/config.gopkg/config/config_test.gopkg/registry/apps/application/rest.gopkg/registry/apps/application/rest_helmrelease_spec_test.gopkg/registry/apps/application/rest_timeout_test.go
💤 Files with no reviewable changes (1)
- cmd/cozystack-operator/main_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/registry/apps/application/rest_helmrelease_spec_test.go`:
- Around line 226-231: The test currently dereferences hr.Spec.Install.Timeout
and hr.Spec.Upgrade.Timeout without guarding against nils; update the test
around the assertions (the block that checks hr.Spec.Install.Timeout.Duration
and hr.Spec.Upgrade.Timeout.Duration) to first verify hr.Spec.Install and
hr.Spec.Upgrade are non-nil and that their Timeout fields are non-nil, and call
t.Errorf with clear messages if any are nil, only then compare Timeout.Duration
to tc.wantInstall / tc.wantUpgrade; this will prevent panics if
convertApplicationToHelmRelease regresses and returns nil Install/Upgrade.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 979b8780-0a1b-466b-99be-84653fc42774
📒 Files selected for processing (5)
pkg/cmd/server/start.gopkg/cmd/server/start_test.gopkg/registry/apps/application/rest.gopkg/registry/apps/application/rest_helmrelease_spec_test.gopkg/registry/apps/application/rest_timeout_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cmd/server/start.go
- pkg/registry/apps/application/rest_timeout_test.go
- pkg/registry/apps/application/rest.go
0f40ed9 to
18d8d58
Compare
18d8d58 to
6658a48
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the core change is correct and the operator-parity claim verifies line by line, but the branch ships a false factual claim in three test comments, a regressed code comment, unformatted new code, and a PR body that describes an intermediate version of the code.
Business context: follow-up to #2509 — cozystack-api's convertApplicationToHelmRelease was the remaining HelmRelease-generating path with hardcoded Interval: 5m / Remediation{Retries:-1}; this brings it to flag parity with cozystack-operator (same names, defaults, validation, generated HR shape).
Blockers
B1: the XValidation justification in the Remediation-guard comments is false
File: pkg/registry/apps/application/rest_helmrelease_spec_test.go:100, pkg/registry/apps/application/rest_timeout_test.go:117 (mirrored from internal/operator/package_reconciler_test.go:125)
Issue: the comments claim "helm-controller's XValidation rule rejects Strategy.Name=RetryOnFailure with RetryInterval set alongside a Remediation entry, so a future re-introduction of Remediation{Retries:-1} would silently break every HR."
Evidence: the deployed helm-controller CRD (internal/fluxinstall/manifests/fluxcd.yaml) contains exactly two strategy-related rules — .retryInterval cannot be set when .name is 'RemediateOnFailure' (line 2598, InstallStrategy) and .retryInterval can only be set when .name is 'RetryOnFailure' (line 3098, UpgradeStrategy). No rule forbids Strategy alongside Remediation; an object carrying both passes CRD validation. The claim is also internally inconsistent — an XValidation rejection would be a loud Create/Update error, not a silent break.
Impact: the guard itself (Remediation stays nil) is worth keeping, but future readers will trust a non-existent safety net and may reason about migrations from it.
Fix: reword the justification to something verifiable (single retry path, operator parity). Since this PR copies the wording from the operator test, fix package_reconciler_test.go:125 in the same pass.
B2: rest.go comment drops tenant-rd from the annotation carriers
File: pkg/registry/apps/application/rest.go:1511
Issue: the new comment says only kubernetes-rd carries release.cozystack.io/helm-install-timeout; the comment this PR deletes correctly listed kubernetes-rd and tenant-rd (with the seaweedfs-db CNPG rationale).
Evidence: packages/system/tenant-rd/cozyrds/tenant.yaml:17 carries release.cozystack.io/helm-install-timeout: "15m".
Fix: restore the accurate carrier list.
B3: new test file is not gofmt-clean
File: pkg/registry/apps/application/rest_helmrelease_spec_test.go (TestConvertApplicationToHelmRelease_PerAppTimeoutOverridesGlobal struct fields)
Evidence: gofmt -d shows misaligned field alignment in the case-table struct. cmd/cozystack-operator/main_test.go, which this PR also touches, carries the same pre-existing misalignment in TestParsePlatformSourceURL.
Impact: CI has no gofmt gate (pre-commit only runs make generate), so this lands silently.
Fix: gofmt -w both files.
B4: PR body describes an intermediate version of the code
Issue: (a) the body states "When both per-app and global are zero, the field stays nil and flux defaults apply" — that matched commit a5f580424, but the final code sets Install/Upgrade.Timeout unconditionally; (b) "No new code path runs until an admin explicitly sets a flag" is not accurate: with stock defaults every api-generated HR changes shape, and the effective install/upgrade timeout grows from the implicit flux default (5m, field was nil) to an explicit 10m.
Impact: the timeout doubling is most likely the desired outcome (it matches the operator path), but it is a behavior change shipped as "no change" — the recorded rationale must match what ships.
Fix: update the body: the strategy switch section already discloses the retry-cadence change, add the timeout-default change alongside it and drop the stays-nil paragraph.
Non-blocking follow-ups
- The unresolved suggestion about extracting repeated duration parsing appears to be addressed by
parseAndValidateHelmReleaseFlags()— worth a short reply on the thread so it can be resolved.
Verified while reviewing: flag names/defaults/validation match cozystack-operator verbatim; validation runs in Complete() before any I/O; Update fully replaces the HR spec, so the existing fleet of Remediation{Retries:-1} HRs migrates passively with no CRD-validation conflicts (no rule forbids the transition); the only production ReleaseConfig constructor is start.go with flag-validated values, so zero durations are unreachable outside hand-built test configs; the operator main.go change is a pure extraction (same messages, same behavior); build, vet, and the touched packages' tests are green on the rebased branch.
| if hr.Spec.Install.Strategy.RetryInterval == nil || hr.Spec.Install.Strategy.RetryInterval.Duration != 17*time.Second { | ||
| t.Errorf("Install.Strategy.RetryInterval = %v, want 17s", hr.Spec.Install.Strategy.RetryInterval) | ||
| } | ||
| // Remediation must remain nil: helm-controller's XValidation rule |
There was a problem hiding this comment.
The deployed helm-controller CRD has no rule rejecting Strategy alongside Remediation — only .retryInterval cannot be set when .name is 'RemediateOnFailure' (InstallStrategy) and .retryInterval can only be set when .name is 'RetryOnFailure' (UpgradeStrategy). An HR carrying both Strategy{RetryOnFailure, RetryInterval} and Remediation{Retries:-1} passes CRD validation. The guard is still worth keeping; the justification needs rewording (single retry path / operator parity). Same wording lives in package_reconciler_test.go:125 — worth fixing there in the same pass.
There was a problem hiding this comment.
Fixed in af4c15c. You're right — the deployed helm-controller CRD has no rule forbidding a Strategy alongside a Remediation entry, so the "silently break every HR" claim was false. Reworded the justification in all three files (rest_helmrelease_spec_test.go, rest_timeout_test.go, internal/operator/package_reconciler_test.go) to the real reason Remediation stays nil: retries run through the single Strategy.RetryInterval path, matching cozystack-operator's PackageReconciler.
| // from release.cozystack.io/helm-install-timeout on the | ||
| // ApplicationDefinition at startup) wins over the global defaults | ||
| // (HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout). Today only | ||
| // kubernetes-rd carries the annotation because the Kubernetes |
There was a problem hiding this comment.
tenant-rd also carries this annotation (packages/system/tenant-rd/cozyrds/tenant.yaml:17, 15m, for the seaweedfs-db CNPG bootstrap). The comment this PR replaces listed both carriers — please restore the accurate list.
There was a problem hiding this comment.
Fixed in b2078de. Restored the accurate two-carrier list — kubernetes-rd and tenant-rd (tenant-rd/cozyrds/tenant.yaml:17 sets the 15m annotation for the seaweedfs-db CNPG bootstrap), with the original rationale for both.
|
Aleksei Sviridkin (@lexfrei) thanks for the thorough pass — all four blockers addressed:
Non-blocking follow-up #1 (duration-parse extraction) is addressed by |
|
CodeRabbit (@coderabbitai) all three nitpicks applied:
|
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✨ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Lift parsePositiveDuration from cmd/cozystack-operator/main.go into pkg/config so cozystack-api can reuse the same startup validator without duplication. Operator main.go now imports and delegates; the table-driven test moves to pkg/config/config_test.go. Extend ReleaseConfig with the five HelmRelease generation knobs the cozystack-operator already exposes via flags (PR #2509): HelmReleaseInterval, HelmReleaseRetryInterval, HelmReleaseInstallTimeout, HelmReleaseUpgradeTimeout, HelmReleaseMaxHistory. The existing per-Application HelmInstallTimeout (set from the release.cozystack.io/helm-install-timeout annotation) keeps its override semantics - it still wins over the new global Install/Upgrade timeout defaults when set. No behaviour change in this commit; the new fields are wired up in follow-up commits. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Wire five flags into cozystack-api that mirror the cozystack-operator flags introduced in PR #2509: --helmrelease-interval (default 5m) --helmrelease-retry-interval (default 30s) --helmrelease-install-timeout (default 10m) --helmrelease-upgrade-timeout (default 10m) --helmrelease-max-history (default 5) CozyServerOptions.Complete() validates each duration via config.ParsePositiveDuration so a misconfigured operator restarts loudly instead of waiting until the first Application is created, and rejects MaxHistory < 0 (0 is valid: unlimited per Helm semantics, matching the operator). Validated values are written into every ReleaseConfig built from the ApplicationDefinitionList. The per-Application HelmInstallTimeout (annotation-driven) still wins over the new global Install/Upgrade timeout defaults; this commit only plumbs the values - the consumer in convertApplicationToHelmRelease lands in the next commit. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
convertApplicationToHelmRelease now builds the HelmRelease spec from ReleaseConfig instead of hardcoding values, bringing it in lockstep with cozystack-operator's PackageReconciler.buildHelmReleaseSpec (PR #2509). The drift between the two HelmRelease-generating paths was the rationale for this follow-up. Changes to the generated spec: - Spec.Interval reads ReleaseConfig.HelmReleaseInterval (was hardcoded 5m). - Spec.MaxHistory set as &maxHistory pointer from ReleaseConfig.HelmReleaseMaxHistory; 0 survives as pointer-to-0 (unlimited per Helm semantics) - mirrors the operator's TestBuildHelmReleaseSpecZeroMaxHistory. - Install.Strategy{Name: RetryOnFailure, RetryInterval} replaces Install.Remediation{Retries: -1}. Same for Upgrade.Strategy. Functionally equivalent to "retry forever, never remediate" for the api-side path (which also never relied on remediation firing) but decouples failed-install retry timing from Spec.Interval. - Install.Remediation / Upgrade.Remediation stay nil: helm-controller's XValidation rejects RetryOnFailure + RetryInterval alongside a Remediation entry, so reintroducing Retries:-1 "for safety" would silently break every HR. Install/Upgrade.Timeout precedence: - per-Application HelmInstallTimeout (annotation-driven) wins when non-zero (preserves the kubernetes-rd opt-in path); - otherwise HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout globals apply; - when both are zero, the field stays nil and flux defaults apply (preserves the legacy code path for tests that hand-construct ReleaseConfig without populating the new globals). Tests: - rest_timeout_test.go: Remediation.Retries == -1 assertions replaced with Strategy.Name == RetryOnFailure + Remediation == nil, matching the operator's spec test. - rest_helmrelease_spec_test.go: new file mirroring TestBuildHelmReleaseSpec / TestBuildHelmReleaseSpecZeroMaxHistory from internal/operator/package_reconciler_test.go, plus a third case asserting per-Application HelmInstallTimeout overrides the global Install/Upgrade timeout defaults. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…tor tests
Production change:
- convertApplicationToHelmRelease now sets Install/Upgrade.Timeout
unconditionally (matching cozystack-operator's buildHelmReleaseSpec).
The previous "stay nil if zero" branch was a leaky test shim — it kept
fixtures with empty ReleaseConfig{} green at the cost of api/operator
drift.
Test changes:
- newRESTForTimeout seeds the new HelmRelease* globals with
production-shaped defaults so the unset-annotation case exercises
"global default applies" rather than "field stays nil".
- TestConvertApplicationToHelmRelease_PerAppTimeoutOverridesGlobal is
table-driven and now covers asymmetric globals (install != upgrade)
with and without per-app override — guards against a regression that
only assigns one side under override.
- New start_test.go: table-driven validator coverage for each
malformed/non-positive duration flag, the negative MaxHistory branch,
the MaxHistory=0 unlimited branch, asymmetric timeout acceptance, and
a smoke test that NewCozyServerOptions defaults validate.
Refactor:
- Extracted parseAndValidateHelmReleaseFlags from Complete() so the
flag-validation portion is unit-testable without kubernetes I/O.
Documentation:
- Reworded --helmrelease-install/upgrade-timeout help so admins reading
--help see the dual-purpose annotation explicitly (one annotation
overrides both timeouts together).
- Dropped PR-#2509 references and "preserving the pre-#2509 behaviour
for tests" comments — kept the technical rationale (Strategy.Name
vs Remediation.Retries:-1 + 5m Interval coupling) on its own merits.
Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…parity The XValidation justification was factually wrong: the deployed helm-controller CRD has no rule rejecting a Strategy alongside a Remediation entry (only .retryInterval is gated on Strategy.Name), so the claimed "silently break every HR" safety net does not exist. Reword the comment in all three test files to the real reason Remediation stays nil: retries are driven by a single Strategy.RetryInterval path, matching cozystack-operator's PackageReconciler.buildHelmReleaseSpec. Address review feedback from lexfrei on rest_helmrelease_spec_test.go, rest_timeout_test.go and internal/operator/package_reconciler_test.go. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The convertApplicationToHelmRelease comment listed only kubernetes-rd as carrying release.cozystack.io/helm-install-timeout, dropping tenant-rd. tenant-rd/cozyrds/tenant.yaml sets the 15m annotation for the seaweedfs-db CNPG bootstrap whose first reconcile exceeds the flux default. Restore the accurate two-carrier list and rationale. Address review feedback from lexfrei on rest.go. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
gofmt -w on the two touched test files: the case-table struct fields in rest_helmrelease_spec_test.go and main_test.go were misaligned, and main_test.go carried a trailing blank line. CI runs no gofmt gate (pre-commit only runs make generate), so this would otherwise land unformatted. Address review feedback from lexfrei. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
HelmReleaseMaxHistory carried omitempty, so a value of 0 (which Helm treats as "unlimited") would be dropped on marshal and become indistinguishable from an unset field. The runtime path (start.go always populates it from --helmrelease-max-history) is safe today, but drop omitempty so the explicit "unlimited" intent survives if ReleaseConfig is ever serialized. Address review feedback from coderabbitai on config.go. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
The ParsePositiveDuration table exercised all error inputs but did not distinguish which branch fired. Add an errMatch field so the table pins "must be > 0" for zero/negative and "invalid duration" for malformed/empty inputs, mirroring TestParseHelmInstallTimeoutAnnotation and keeping the two parsers' diagnostics from drifting. Address review feedback from coderabbitai on config_test.go. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
release.cozystack.io/helm-install-timeout sets both Install.Timeout and Upgrade.Timeout to one value. Add release.cozystack.io/helm-upgrade-timeout so an ApplicationDefinition can override only Upgrade.Timeout — letting a kind keep a short install budget but a longer upgrade budget (or vice versa). When set it wins over the value the install annotation would otherwise apply to the upgrade side. - config: add HelmUpgradeTimeoutAnnotation + ReleaseConfig.HelmUpgradeTimeout; generalize ParseHelmInstallTimeoutAnnotation to ParseHelmTimeoutAnnotation since it now parses both annotations (identical Flux unit validation). - server: parse the new annotation at startup with the same loud-fail behavior; update --helmrelease-upgrade-timeout help. - rest: apply HelmUpgradeTimeout after the install-annotation copy so the upgrade-specific value wins; restore the accurate carrier comment. - tests: cover the upgrade-only override, the asymmetric precedence, and the install-sets-both regression guard. Behavior is unchanged for every existing ApplicationDefinition (none set the new annotation today). Address review feedback from coderabbitai on rest.go. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
ca00511 to
d405cdc
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — all four blockers from my prior review are resolved and verified at HEAD, and the new asymmetric upgrade-timeout feature is correct and well-tested. One non-blocking validation-parity gap remains.
Business context: follow-up to #2509 — brings cozystack-api's convertApplicationToHelmRelease to flag parity with cozystack-operator (same flag names/defaults/validation, same generated HR shape), and adds an asymmetric per-Application upgrade-timeout annotation.
Resolved since my last review
- B1 (false XValidation justification) — the three test files no longer claim helm-controller rejects a
Strategyalongside aRemediationentry; the body now states plainly it is "a parity/clarity guard, not a CRD-validation one". Verified no XValidation wording remains. - B2 (rest.go dropped tenant-rd) — the carrier comment now reads "kubernetes-rd and tenant-rd carry helm-install-timeout today" (
rest.go:1534). Restored. - B3 (gofmt) — all 10 touched Go files are
gofmt-clean. - B4 (body described intermediate code) — the body now carries an explicit "Timeout-default change" / "With stock defaults, every api-generated HR changes shape … not a no-op" section, and the "stay nil when zero" paragraph is gone.
New feature (helm-upgrade-timeout) — reviewed
- Precedence in
convertApplicationToHelmRelease(rest.go:1542-1550) is correct: install = install-annotation > global default; upgrade = upgrade-annotation > install-annotation > global default. Matches the documented semantics. - Annotations are parsed at registration (
start.go:280-299) withcrd.Name-scoped errors → loud startup failure.MaxHistory=0survives YAML marshal (noomitempty); the timeout fields keepomitemptycorrectly (unset → flux default). - Coverage is solid:
TestConvertApplicationToHelmRelease_UpgradeTimeoutAnnotationexercises upgrade-only override, asymmetric precedence over the install annotation, and the "install override alone still sets both" regression guard. Touched-package tests andgo vetare green at HEAD. - The earlier suggestion to extract the repeated duration parsing is addressed by
parseAndValidateHelmReleaseFlags()— safe to resolve that thread.
Non-blocking follow-up
- Flag vs annotation unit-validation asymmetry. The
--helmrelease-*duration flags go throughconfig.ParsePositiveDuration(config.go:77), which istime.ParseDuration+ a> 0check only — it acceptsns/us/µs. The annotation path (ParseHelmTimeoutAnnotation) rejects exactly those viahelmTimeoutPattern, with a comment explaining why ("rejected by Flux … instead of silently … failing later at HelmRelease admission"). So--helmrelease-interval=500uspasses startup validation, then every generated HR'sSpec.Intervalis rejected at Flux admission — the one case where this PR's fail-fast-at-startup contract doesn't hold. It fails loudly (not silently) and only on an operationally-nonsensical sub-ms value, so it's not blocking.ParsePositiveDurationis shared withcozystack-operator, which has the identical gap, so the parity-preserving fix is to apply the Flux-unit pattern in the shared validator rather than only on the api side.
Why
Follow-up to #2509. PR #2509 exposed five HelmRelease generation knobs as
cozystack-operatorflags (--helmrelease-{interval,retry-interval,install-timeout,upgrade-timeout,max-history}). The other HelmRelease-generating path —cozystack-api'sconvertApplicationToHelmReleaseinpkg/registry/apps/application/rest.go— was deliberately left out of #2509's scope and was flagged in review:So today the api-side path hardcodes:
Spec.Interval = 5mInstall.Remediation{Retries: -1}+Upgrade.Remediation{Retries: -1}StrategyMaxHistoryOnly
HelmInstallTimeout(per-Application annotation override) flows throughReleaseConfig. This PR brings the api-side to feature parity with the operator: same flag names, same defaults, same validation, same generated HelmRelease shape.What
New cozystack-api flags
Names, types, defaults, and validation match the operator's flags introduced in #2509 verbatim, so both binaries can be tuned with the same vocabulary.
--helmrelease-interval5mSpec.Interval--helmrelease-retry-interval30sSpec.{Install,Upgrade}.Strategy.RetryInterval--helmrelease-install-timeout10mSpec.Install.Timeout--helmrelease-upgrade-timeout10mSpec.Upgrade.Timeout--helmrelease-max-history5Spec.MaxHistoryValidated in
CozyServerOptions.Complete()viaconfig.ParsePositiveDurationso a misconfigured server restarts loudly instead of waiting until the first Application is created.MaxHistory < 0is rejected;0is honored (unlimited per Helm semantics, matching the operator).Strategy switch
Install.Strategy.NameandUpgrade.Strategy.Nameare now set toRetryOnFailure(withRetryIntervalfrom the new flag) instead ofInstall/Upgrade.Remediation{Retries: -1}. Functionally equivalent for this path (which never relied on remediation firing), but decouples failed-install retry timing fromSpec.Intervalthe same way the operator change did.Install.Remediation/Upgrade.Remediationstay nil: retries run through the singleStrategy.RetryIntervalpath, matching cozystack-operator'sPackageReconciler. ReintroducingRemediation{Retries:-1}"for safety" would add a second, conflicting retry mechanism and break parity with the operator path. A regression test guards this. (The deployed helm-controller CRD has no rule forbidding aStrategyalongside aRemediationentry — this is a parity/clarity guard, not a CRD-validation one.)Per-Application timeout override is preserved (and extended)
The
release.cozystack.io/helm-install-timeoutannotation onApplicationDefinition(HelmInstallTimeoutinReleaseConfig) still wins over the new globalHelmReleaseInstallTimeout/HelmReleaseUpgradeTimeoutdefaults when set, applying to bothInstall.TimeoutandUpgrade.Timeout. kubernetes-rd and tenant-rd carry it today (Kamaji bootstrap; seaweedfs-db CNPG bootstrap).A new
release.cozystack.io/helm-upgrade-timeoutannotation overrides onlyUpgrade.Timeout, winning over the value the install annotation would otherwise apply — so a kind can carry an asymmetric install/upgrade budget. NoApplicationDefinitionsets it today, so behavior is unchanged for every existing kind.Install.TimeoutandUpgrade.Timeoutare now always populated (from a per-app override or the global default), matching the operator'sbuildHelmReleaseSpec. The previous "stay nil when zero" branch was removed — it was a test shim that kept empty-ReleaseConfig{}fixtures green at the cost of api/operator drift.Shared validator
parsePositiveDurationis lifted fromcmd/cozystack-operator/main.gointopkg/config(exported asParsePositiveDuration). Bothcozystack-operatorandcozystack-apimains now share one validator with identical reject rules (zero / negative / malformed / empty). The table-driven test moves with it. The annotation parser is shared the same way:ParseHelmTimeoutAnnotationvalidates both the install- and upgrade-timeout annotation values against Flux's unit rules.Production safety
Strategy.Name=RetryOnFailure+RetryInterval, an explicitMaxHistory, and an explicitInstall/Upgrade.Timeout. This is the intended parity with the operator path, not a no-op.Install/Upgrade.Timeoutis now always set, the effective install/upgrade timeout for api-generated HRs grows from the implicit flux default (5m, field was previously nil) to the explicit--helmrelease-{install,upgrade}-timeoutdefault of10m. This is desired — it matches the operator path.Spec.Interval) to 30s (RetryIntervaldefault). Healthy-release reconcile cadence is unchanged at 5m.HelmInstallTimeoutoverride semantics intact — kubernetes-rd's long Kamaji bootstrap path continues to work; tenant-rd's seaweedfs-db bootstrap budget is unchanged.Verification
go build ./...cleango vet ./...cleango test ./pkg/config/... ./pkg/cmd/server/... ./pkg/registry/apps/application/... ./cmd/cozystack-operator/... ./internal/operator/...— all passTestConvertApplicationToHelmRelease_BuildsSpecFromConfig— full HR shape from config (mirrorsTestBuildHelmReleaseSpec)TestConvertApplicationToHelmRelease_ZeroMaxHistory—MaxHistory=0survives as pointer-to-0 (mirrorsTestBuildHelmReleaseSpecZeroMaxHistory)TestConvertApplicationToHelmRelease_PerAppTimeoutOverridesGlobal— install-annotation override wins over globals (symmetric + asymmetric)TestConvertApplicationToHelmRelease_UpgradeTimeoutAnnotation— upgrade-only override, asymmetric precedence over the install annotation, and the install-sets-both regression guardTestConvertApplicationToHelmRelease_AppliesReleaseConfigTimeout—Remediation.Retries == -1assertions replaced withStrategy.Name == RetryOnFailure+Remediation == nilTestParsePositiveDuration/TestParseHelmTimeoutAnnotation— flag and annotation validation, including which error branch firespkg/apiserver(randfill panic) andpkg/lineagereproduce onmainwithout these changes — not introduced here.Test plan
Strategy.Name=RetryOnFailureandRetryInterval=30sInstall/Upgrade.Timeout=15m(or whatever its annotation says) — overrides the global defaultFollowups (deliberately not bundled)
packages/core/installer/values.yamlknobs, but for the api server. Touches a different chart (packages/core/cozystack-apior wherever the api Deployment lives). Separate PR keeps the diff focused on the Go side.Summary by CodeRabbit