proposal: ApplicationDefinition multi-version conversion#6
proposal: ApplicationDefinition multi-version conversion#6
Conversation
Introduces a draft design for versioning the API surface of ApplicationDefinitions while keeping values.yaml as the storage form. Defines per-version ApplicationSchema files, a pair of go-template conversion snippets (to/from storage), a migration controller for self-recovery, and a chart-side version guard. Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
📝 WalkthroughWalkthroughIntroduces a design proposal document detailing an Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![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 a design proposal for multi-version conversion of ApplicationDefinition in Cozystack, enabling versioned API surfaces for values.yaml through Go-template-based conversions and a background migration controller. The review feedback highlights the need for more robust field access in conversion templates, explicit error handling for 'poison pill' resources in the migration controller, and a more flexible approach to consecutive storage version bumps to ensure long-term upgrade stability.
| replicas: {{ .replicas }} | ||
| maxConnections: {{ index .postgresql.parameters "max_connections" }} |
There was a problem hiding this comment.
While some parts of the example use default (e.g., line 137), lines 134 and 135 access fields directly. If .replicas is missing or .postgresql is nil, the template will either produce invalid YAML (replicas: <no value>) or fail with a rendering error. It is recommended to consistently use default or existence checks for all field accesses to ensure the conversion is robust against partial data.
| if cv not in servedVersions: | ||
| emit Warning: ManualMigrationRequired | ||
| continue | ||
| valuesNew := apply(versions[cv].to, valuesOld) |
There was a problem hiding this comment.
The migration controller pseudo-code should explicitly define how to handle failures during the apply step. Since Go templates can encounter runtime errors (e.g., invalid syntax in generated YAML or logic errors), the controller needs a mechanism to handle 'poison pill' resources—such as logging the error and skipping the resource—to prevent a single failing conversion from stalling the entire migration process.
| in a row. Two consecutive storage bumps would require a transitive | ||
| conversion path that is not supported here. |
There was a problem hiding this comment.
The restriction against consecutive storage bumps creates a significant maintenance burden and upgrade risk. If the storage version is the 'hub', every time it is bumped, all existing legacy version templates must be updated to target the new hub. Without this, users skipping a Cozystack release would find their resources un-migratable. The design should consider if a transitive conversion mechanism (e.g., V1 -> V2 -> V3) is necessary for long-term stability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
design-proposals/application-definition-versioning/README.md (4)
202-212: 💤 Low valueAdd language specifier to pseudocode block.
The migration controller logic is clear, but the code block should specify a language.
📝 Suggested fix
-``` +```python for hr in helmreleases of this kind:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@design-proposals/application-definition-versioning/README.md` around lines 202 - 212, Add a language specifier to the fenced code block containing the migration pseudocode (the block starting with "for hr in helmreleases of this kind:") so the snippet is marked as a language (e.g., ```python) to enable proper syntax highlighting; update the opening fence to include the chosen language without altering the pseudocode lines such as "cv := hr.values._version", "if cv not in servedVersions", "valuesNew := apply(versions[cv].to, valuesOld)", and "patch(hr, valuesNew)".
170-192: 💤 Low valueAdd language specifier to pseudocode block.
The conversion flow logic is sound (storage-as-hub pattern), but the code block should specify a language.
📝 Suggested fix
-``` +```python Write spec_X (any served X):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@design-proposals/application-definition-versioning/README.md` around lines 170 - 192, The pseudocode block for the "Conversion flow" should explicitly declare a language for syntax highlighting; update the fenced code block that currently starts with ``` to include a language tag (e.g., ```python) so the block containing the Write spec_X / Read as Y flow (references: spec_X, values_X, versions[X].to, versions[Y].from, _version, values_storage, HelmRelease) is fenced as a named language; do not alter the pseudocode itself—only change the opening triple-backtick to include the language specifier.
100-108: 💤 Low valueAdd language specifier to code block.
The file layout is clear, but the fenced code block should specify a language for better rendering and tooling support.
📝 Suggested fix
-``` +```text packages/apps/<name>/ ├── values.yaml # storage form, single source of truth🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@design-proposals/application-definition-versioning/README.md` around lines 100 - 108, The fenced code block showing the directory tree (the block that begins with "packages/apps/<name>/" and lists values.yaml, api/v1alpha1.yaml, v1.yaml, and templates/_cozystack-version.tpl) should include a language specifier for proper rendering; update the opening fence from ``` to a specific language hint such as ```text (or ```bash) in README.md so tooling and renderers treat the block as plain text.
287-289: ⚡ Quick winConsider enforcing the storage version bump constraint.
The constraint "A single Cozystack release should not bump storage version twice in a row" is critical for avoiding transitive conversion paths, but it's stated as a "should" without enforcement mechanism.
Consider whether
cozyschema buildor a CI check could enforce this by examining version history across releases, or at minimum, add this to the CI checks list (lines 251-260).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@design-proposals/application-definition-versioning/README.md` around lines 287 - 289, Add an automated enforcement of the "no two consecutive storage version bumps" rule by implementing a check in the existing tooling and/or CI: extend the command "cozyschema build" to inspect the repository's release/version history (tags or stored schema versions) and detect if the most recent release and the previous release both include storage version bumps, failing with a clear error if so, and/or add a CI job (e.g., "check-storage-version-bumps") that runs this same check during PRs; update documentation to reference the new check in the CI checks list and ensure the check uses unique identifiers like storage version fields and release tags to determine consecutive bumps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@design-proposals/application-definition-versioning/README.md`:
- Around line 306-309: Add the missing legacyDefault field to the
ApplicationDefinition examples and schema outputs: update the ApplicationSchema
example (the block showing ApplicationDefinition structure) to include a brief
legacyDefault: <version> entry and reflect that same field in the cozyschema
build outputs section so readers can see where legacyDefault lives; reference
the ApplicationDefinition examples and the generated cozyschema output blocks to
ensure the field name matches exactly ("legacyDefault") and include a short
comment noting the open question if needed.
---
Nitpick comments:
In `@design-proposals/application-definition-versioning/README.md`:
- Around line 202-212: Add a language specifier to the fenced code block
containing the migration pseudocode (the block starting with "for hr in
helmreleases of this kind:") so the snippet is marked as a language (e.g.,
```python) to enable proper syntax highlighting; update the opening fence to
include the chosen language without altering the pseudocode lines such as "cv :=
hr.values._version", "if cv not in servedVersions", "valuesNew :=
apply(versions[cv].to, valuesOld)", and "patch(hr, valuesNew)".
- Around line 170-192: The pseudocode block for the "Conversion flow" should
explicitly declare a language for syntax highlighting; update the fenced code
block that currently starts with ``` to include a language tag (e.g., ```python)
so the block containing the Write spec_X / Read as Y flow (references: spec_X,
values_X, versions[X].to, versions[Y].from, _version, values_storage,
HelmRelease) is fenced as a named language; do not alter the pseudocode
itself—only change the opening triple-backtick to include the language
specifier.
- Around line 100-108: The fenced code block showing the directory tree (the
block that begins with "packages/apps/<name>/" and lists values.yaml,
api/v1alpha1.yaml, v1.yaml, and templates/_cozystack-version.tpl) should include
a language specifier for proper rendering; update the opening fence from ``` to
a specific language hint such as ```text (or ```bash) in README.md so tooling
and renderers treat the block as plain text.
- Around line 287-289: Add an automated enforcement of the "no two consecutive
storage version bumps" rule by implementing a check in the existing tooling
and/or CI: extend the command "cozyschema build" to inspect the repository's
release/version history (tags or stored schema versions) and detect if the most
recent release and the previous release both include storage version bumps,
failing with a clear error if so, and/or add a CI job (e.g.,
"check-storage-version-bumps") that runs this same check during PRs; update
documentation to reference the new check in the CI checks list and ensure the
check uses unique identifiers like storage version fields and release tags to
determine consecutive bumps.
🪄 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
Run ID: 37d66c26-647e-45d0-8154-6ae87860b41a
📒 Files selected for processing (1)
design-proposals/application-definition-versioning/README.md
| - Missing `_version` on a HelmRelease (legacy resource): the | ||
| `ApplicationDefinition` may declare a `legacyDefault: <version>`. | ||
| The api-server treats absent `_version` as that default on the | ||
| next read or migration. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Show legacyDefault in ApplicationSchema example.
The failure-handling section mentions legacyDefault as a field in ApplicationDefinition (line 307), but this field isn't shown in the ApplicationSchema example (lines 116-159) or in the cozyschema build outputs (lines 239-250).
Consider adding a brief example showing where legacyDefault appears in the ApplicationDefinition structure, even if it's deferred to an open question (line 367-368).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@design-proposals/application-definition-versioning/README.md` around lines
306 - 309, Add the missing legacyDefault field to the ApplicationDefinition
examples and schema outputs: update the ApplicationSchema example (the block
showing ApplicationDefinition structure) to include a brief legacyDefault:
<version> entry and reflect that same field in the cozyschema build outputs
section so readers can see where legacyDefault lives; reference the
ApplicationDefinition examples and the generated cozyschema output blocks to
ensure the field name matches exactly ("legacyDefault") and include a short
comment noting the open question if needed.
1 participant
Summary
Draft design proposal for versioning the API surface of
ApplicationDefinitionwhile keepingvalues.yamlas the storage form.ApplicationSchemafiles underpackages/apps/<name>/api/to/from)for non-storage versions only
storage version in the background
cozyschema buildtool generatesApplicationDefinition,JSON schema, chart annotations, and the guard include from the
per-version sources
UI metadata placement is deliberately left as an open question; one
suggestion is a separate
ApplicationViewresource bound to a schemaby name and version. To be picked up by a follow-up proposal.
Test plan
This is a design-only change. No code lands with this PR.
Summary by CodeRabbit