Remove versions_map logic - #1453
Conversation
WalkthroughReplaces versions_map-driven packaging with direct Helm packaging using discovered CHARTS; removes gen/check-version-map scripts and versions_map data; sets Chart.yaml versions to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Make as Package Makefiles
participant Fix as fix-charts
participant Helm as Helm (package/index)
Note over Make,Helm: New packaging flow (versions_map & scripts removed)
Dev->>Make: make repo
Make->>Fix: run fix-charts (normalize names, set version: 0.0.0)
Make->>Helm: helm package $(CHARTS) -d $(OUT) --version $(COZYSTACK_VERSION)
Helm-->>Make: packaged charts
Make->>Helm: helm repo index $(OUT)
Helm-->>Make: updated index
Note over Make: Removed steps: gen-versions-map, check-version-map, logo aggregation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 Andrei Kvapil (@kvaps), 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 introduces a significant refactoring of the Helm chart versioning and packaging system. The primary goal is to simplify the build pipeline by eliminating a custom Highlights
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring that removes the complex versions_map logic for managing Helm chart versions. It simplifies the build process by packaging all charts from the current state of the repository and setting their versions based on the COZYSTACK_VERSION environment variable. This is a great improvement for maintainability.
My review has identified one high-severity issue that appears to be a regression: the handling of chart icon files has been removed, which will likely lead to broken images in the UI. I've also provided a medium-severity suggestion to re-introduce a CI check to ensure chart metadata files are kept up-to-date, similar to the old check-version-map functionality.
bca8c63 to
e2b9007
Compare
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
e2b9007 to
f871fbd
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/system/etcd-operator/Chart.yaml (1)
1-3: Chart.yaml missing apiVersion (Helm v3 requires apiVersion: v2).This Chart will fail linting/packaging without apiVersion. Add it explicitly.
+apiVersion: v2 name: cozy-etcd-operator -version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process +version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process
🧹 Nitpick comments (19)
packages/system/bootbox/templates/bootbox.yaml (1)
19-19: Confirm intent to include pre-releases and consider an upper bound to avoid unexpected upgradesUsing '>= 0.0.0-0' includes all pre-release versions per Helm/Masterminds semver and will auto-track the latest chart in the repo. If that’s intentional, LGTM. If you want to avoid accidental major bumps, add an upper bound.
Example options:
- version: '>= 0.0.0-0' + version: '>= 0.0.0-0 <1.0.0'Or pin exactly via build-time substitution if reproducibility is required:
- version: '>= 0.0.0-0' + version: '${CHART_VERSION}'Please confirm that the cozystack-extra index uses semver-compatible versions and that tracking pre-releases is desired for bootbox.
packages/system/etcd-operator/Chart.yaml (1)
1-3: Consider adding appVersion for clarity/consistency.If other charts carry appVersion, keep this aligned (set to the etcd-operator image/tag).
apiVersion: v2 name: cozy-etcd-operator version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process +appVersion: "PLACEHOLDER" # optional: set or substitute to operator image versionpackages/apps/vm-instance/Chart.yaml (1)
6-6: Build injects chart version; add CI guard to prevent publishing 0.0.0
- Confirmed: packaging uses --version $(COZYSTACK_VERSION) (packages/{apps,system,library,extra}/Makefile) and COZYSTACK_VERSION is computed from git tags in scripts/common-envs.mk.
- Risk: many Chart.yaml still use version: 0.0.0 placeholders (including packages/apps/vm-instance/Chart.yaml). Add a CI check to fail when COZYSTACK_VERSION is empty or when any Chart.yaml contains 0.0.0 (e.g. fail if rg -nP '^\sversion:\s0.0.0\b' matches).
packages/apps/tcp-balancer/Chart.yaml (1)
4-4: Icon path may not resolve in Helm/Artifact HubAbsolute path “/logos/haproxy.svg” typically won’t render in Helm UIs unless published behind a matching host. Prefer a fully qualified URL or repo-relative raw URL.
Example:
- https://raw.githubusercontent.com////logos/haproxy.svg
packages/system/hetzner-robotlb/Chart.yaml (1)
3-3: Guard against leaking placeholder versions into releases.If a packaging job runs without substitution, multiple charts at 0.0.0 will collide in the Helm index. Ensure CI fails when any Chart.yaml with
version: 0.0.0is about to be published and that the build step reliably substitutes versions.packages/apps/tenant/templates/ingress.yaml (1)
20-20: Consider constraining to a safe major/minor range instead of “anything.”
'>= 0.0.0-0'permits all versions, including pre-releases. If the repo ever publishes breaking or pre-release builds, this will pick them. Recommend pinning to a major (e.g.,^1.0.0) or specific range aligned with your compatibility guarantees.packages/extra/info/Chart.yaml (1)
6-6: Ensure build-time version substitution is enforced in CI.Using
0.0.0is fine as a placeholder, but add/verify a pipeline check that forbids publishing charts still at0.0.0.packages/apps/tenant/Chart.yaml (1)
6-6: Same placeholder-version concern as other charts.Please confirm your packaging flow always replaces
0.0.0and fails otherwise.packages/apps/vm-disk/Chart.yaml (1)
7-7: Quote appVersion for YAML/Helm consistency.Unquoted
0.4.0can be parsed inconsistently; convention is to quote appVersion.Apply this diff:
-appVersion: 0.4.0 +appVersion: "0.4.0"packages/apps/ferretdb/Chart.yaml (1)
7-7: Quote appVersion for consistency.Align with other charts (e.g., seaweedfs) by quoting the version.
Apply this diff:
-appVersion: 2.4.0 +appVersion: "2.4.0"packages/extra/etcd/Chart.yaml (1)
6-6: Verify placeholder substitution and consider adding appVersion.Confirm CI enforces replacing
0.0.0before publish. Optionally addappVersionto aid consumers.packages/apps/virtual-machine/Chart.yaml (1)
6-7: Quote appVersion fields in Chart.yaml filesQuote numeric appVersion values to avoid YAML parsing pitfalls and keep consistency; apply the same change (appVersion: X.Y.Z → appVersion: "X.Y.Z") to these files:
- packages/system/coredns/charts/coredns/Chart.yaml
- packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/Chart.yaml
- packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/charts/common/Chart.yaml
- packages/system/victoria-metrics-operator/charts/victoria-metrics-operator/charts/crds/Chart.yaml
- packages/system/velero/charts/velero/Chart.yaml
- packages/system/telepresence/charts/telepresence/Chart.yaml
- packages/system/redis-operator/charts/redis-operator/Chart.yaml
- packages/system/opencost/charts/opencost/Chart.yaml
- packages/system/nfs-driver/charts/csi-driver-nfs/Chart.yaml
- packages/system/nats/charts/nats/Chart.yaml
- packages/system/monitoring-agents/charts/prometheus-node-exporter/Chart.yaml
- packages/system/monitoring-agents/charts/metrics-server/Chart.yaml
- packages/system/monitoring-agents/charts/kube-state-metrics/Chart.yaml
- packages/system/monitoring-agents/charts/fluent-bit/Chart.yaml
- packages/system/keycloak-operator/charts/keycloak-operator/Chart.yaml
- packages/system/postgres-operator/charts/cloudnative-pg/Chart.yaml
- packages/system/hetzner-robotlb/charts/robotlb/Chart.yaml
- packages/system/mariadb-operator/charts/mariadb-operator/Chart.yaml
- packages/system/goldpinger/charts/goldpinger/Chart.yaml
- packages/system/external-dns/charts/external-dns/Chart.yaml
- packages/system/mariadb-operator/charts/mariadb-operator/charts/mariadb-operator-crds/Chart.yaml
- packages/system/ingress-nginx/charts/ingress-nginx/Chart.yaml
- packages/system/cozy-proxy/charts/cozy-proxy/Chart.yaml
- packages/system/clickhouse-operator/charts/altinity-clickhouse-operator/Chart.yaml
- packages/system/dashboard/charts/kubeapps/Chart.yaml
- packages/system/dashboard/charts/kubeapps/charts/redis/Chart.yaml
- packages/system/dashboard/charts/kubeapps/charts/common/Chart.yaml
- packages/system/cilium/charts/cilium/Chart.yaml
- packages/system/dashboard/charts/kubeapps/charts/redis/charts/common/Chart.yaml
- packages/system/capi-operator/charts/cluster-api-operator/Chart.yaml
- packages/apps/ferretdb/Chart.yaml
- packages/apps/vm-instance/Chart.yaml
- packages/apps/vm-disk/Chart.yaml
- packages/apps/kubernetes/Chart.yaml
- packages/apps/virtual-machine/Chart.yaml
pkg/registry/apps/application/rest.go (1)
1031-1036: Make interval configurable and re-evaluate unlimited retries (-1)
- Verification: retries = -1 is documented as “unlimited retries” and is supported, but can cause unbounded upgrade loops; some remediation failure paths may still stop retries and require manual intervention.
- Action items:
- Read Interval (default 5m) and Upgrade.Remediation.Retries from r.releaseConfig instead of hard-coding.
- Avoid defaulting to -1 unless you have alerting and operational controls; prefer a finite retry count or a configurable max backoff/circuit-breaker.
- If keeping -1, add alerting/silencing and a max backoff to prevent repeated churn on persistent failures.
Location: pkg/registry/apps/application/rest.go (around lines 1031–1036)
packages/system/Makefile (1)
1-3: Declare phony targets for make hygiene.Add
.PHONY: repo fix-chartsto avoid file/target name collisions.+ .PHONY: repo fix-charts OUT=../../_out/repos/system CHARTS := $(shell find . -maxdepth 2 -name Chart.yaml | awk -F/ '{print $$2}')packages/apps/Makefile (2)
4-4: Declare phony targets for make hygiene.include ../../scripts/common-envs.mk +.PHONY: repo fix-charts
2-2: CHARTS discovery — OK for current layout. find -maxdepth 2 returned: bucket, clickhouse, ferretdb, http-cache, kafka, kubernetes, mysql, nats, postgres, rabbitmq, redis, tcp-balancer, tenant, virtual-machine, vm-disk, vm-instance, vpn — each dir contains Chart.yaml, so awk -F/ '{print $$2}' is safe; optional: switch to dirname-based discovery for robustness.packages/library/Makefile (1)
1-5: Add phony targets (and optional clean) to satisfy checkmake.+ .PHONY: repo fix-charts clean OUT=../../_out/repos/library CHARTS := $(shell find . -maxdepth 2 -name Chart.yaml | awk -F/ '{print $$2}') include ../../scripts/common-envs.mk + +clean: + rm -rf "$(OUT)"packages/extra/Makefile (2)
1-5: Add phony targets (and optional clean) to satisfy checkmake.+ .PHONY: repo fix-charts clean OUT=../../_out/repos/extra CHARTS := $(shell find . -maxdepth 2 -name Chart.yaml | awk -F/ '{print $$2}') include ../../scripts/common-envs.mk + +clean: + rm -rf "$(OUT)"
11-12: Guard sed against leading spaces or commented keys.Anchoring with
^name:and^version:skips lines with indentation. Slightly more robust:- find . -maxdepth 2 -name Chart.yaml | awk -F/ '{print $$2}' | while read i; do sed -i -e "s/^name: .*/name: $$i/" -e "s/^version: .*/version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process/g" "$$i/Chart.yaml"; done + find . -maxdepth 2 -name Chart.yaml | awk -F/ '{print $$2}' | while read i; do \ + sed -i -E \ + -e "s/^[[:space:]]*name:[[:space:]].*/name: $$i/" \ + -e "s/^[[:space:]]*version:[[:space:]].*/version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process/" \ + "$$i/Chart.yaml"; \ + done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
Makefile(0 hunks)hack/gen_versions_map.sh(0 hunks)hack/package_chart.sh(0 hunks)packages/apps/Makefile(1 hunks)packages/apps/bucket/Chart.yaml(1 hunks)packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/ferretdb/Chart.yaml(1 hunks)packages/apps/http-cache/Chart.yaml(1 hunks)packages/apps/kafka/Chart.yaml(1 hunks)packages/apps/kubernetes/Chart.yaml(1 hunks)packages/apps/mysql/Chart.yaml(1 hunks)packages/apps/nats/Chart.yaml(1 hunks)packages/apps/postgres/Chart.yaml(1 hunks)packages/apps/rabbitmq/Chart.yaml(1 hunks)packages/apps/redis/Chart.yaml(1 hunks)packages/apps/tcp-balancer/Chart.yaml(1 hunks)packages/apps/tenant/Chart.yaml(1 hunks)packages/apps/tenant/templates/ingress.yaml(1 hunks)packages/apps/versions_map(0 hunks)packages/apps/virtual-machine/Chart.yaml(1 hunks)packages/apps/vm-disk/Chart.yaml(1 hunks)packages/apps/vm-instance/Chart.yaml(1 hunks)packages/apps/vpn/Chart.yaml(1 hunks)packages/core/installer/images/cozystack/Dockerfile(0 hunks)packages/extra/Makefile(1 hunks)packages/extra/bootbox/Chart.yaml(1 hunks)packages/extra/etcd/Chart.yaml(1 hunks)packages/extra/info/Chart.yaml(1 hunks)packages/extra/ingress/Chart.yaml(1 hunks)packages/extra/monitoring/Chart.yaml(1 hunks)packages/extra/seaweedfs/Chart.yaml(1 hunks)packages/extra/versions_map(0 hunks)packages/library/Makefile(1 hunks)packages/library/cozy-lib/Chart.yaml(1 hunks)packages/system/Makefile(1 hunks)packages/system/bootbox/templates/bootbox.yaml(1 hunks)packages/system/etcd-operator/Chart.yaml(1 hunks)packages/system/hetzner-robotlb/Chart.yaml(1 hunks)packages/system/kubeovn-plunger/Chart.yaml(1 hunks)packages/system/kubeovn-webhook/Chart.yaml(1 hunks)pkg/registry/apps/application/rest.go(2 hunks)
💤 Files with no reviewable changes (6)
- Makefile
- packages/extra/versions_map
- packages/core/installer/images/cozystack/Dockerfile
- hack/package_chart.sh
- packages/apps/versions_map
- hack/gen_versions_map.sh
🧰 Additional context used
🪛 checkmake (0.2.2)
packages/extra/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
packages/library/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🔇 Additional comments (21)
packages/apps/rabbitmq/Chart.yaml (2)
7-7: Verify appVersion aligns with the image tag used by this chart.Avoid drift between appVersion and the container image tag in values/templates.
Run:
6-6: No guard needed — packaging injects the chart versionMakefiles call
helm packagewith--version $(COZYSTACK_VERSION)in: packages/system/Makefile, packages/library/Makefile, packages/extra/Makefile, packages/apps/Makefile.packages/apps/http-cache/Chart.yaml (1)
7-7: Align image tag with appVersion to avoid drift.Ensure values/templates use .Chart.AppVersion (or derive image.tag from it) so appVersion stays authoritative.
Search for image tag usage to confirm:
packages/apps/tcp-balancer/Chart.yaml (1)
6-6: Fail packaging if any Chart.yaml still has version 0.0.0Makefiles (packages/*/Makefile) call
helm package ... --version $(COZYSTACK_VERSION)so the build expects COZYSTACK_VERSION to be injected, but many Chart.yaml (including packages/apps/tcp-balancer/Chart.yaml) still containversion: 0.0.0and the Makefiles include afix-chartsstep that writes that placeholder. Ensure CI sets COZYSTACK_VERSION before packaging and add a CI check that aborts packaging when any packages/**/Chart.yaml containsversion: 0.0.0(e.g.rg -nP '^\s*version:\s*0\.0\.0\b' packages/**/Chart.yaml && exit 1).packages/apps/vpn/Chart.yaml (1)
6-7: Ensure appVersion drives the deployed image tag (or pin image.tag to 1.8.1)Chart.yaml sets appVersion: "1.8.1", but I couldn't find any image/tag configuration referencing .Chart.AppVersion in this chart — .Chart.AppVersion is only used for app.kubernetes.io/version (packages/apps/vpn/templates/_helpers.tpl) and values.yaml has no image settings. Either make the chart’s image.tag use .Chart.AppVersion, or explicitly set image.tag = "1.8.1" to avoid drift.
packages/extra/seaweedfs/Chart.yaml (1)
6-8: LGTM, consistent appVersion quoting.Chart version placeholder plus quoted appVersion looks good and consistent.
packages/apps/nats/Chart.yaml (1)
6-7: LGTM on placeholder + appVersionChart version placeholder with appVersion "1.4.1" looks consistent.
Please confirm CI injects a real chart version before packaging (same verification script as suggested in bootbox).
packages/apps/clickhouse/Chart.yaml (1)
6-7: LGTM on placeholder + appVersionNo issues spotted; appVersion is quoted and semantically correct.
packages/apps/kafka/Chart.yaml (1)
6-7: LGTM on placeholder + appVersionLooks good; appVersion is quoted.
packages/apps/mysql/Chart.yaml (1)
6-7: LGTM on placeholder + appVersionNo issues; appVersion is quoted.
packages/apps/bucket/Chart.yaml (1)
6-7: LGTM on placeholder + appVersionLooks consistent with the new build-time versioning approach.
packages/extra/bootbox/Chart.yaml (1)
6-6: Build-time version injection present — ensure COZYSTACK_VERSION is always populatedMakefiles call "helm package ... --version $(COZYSTACK_VERSION)" and scripts/common-envs.mk derives COZYSTACK_VERSION from git tags; Chart.yaml files keep the 0.0.0 placeholder and are expected to be overridden at package time. Ensure CI produces a tag (or fail/assign a unique fallback) so COZYSTACK_VERSION is never empty before packaging.
Relevant locations: scripts/common-envs.mk (COZYSTACK_VERSION logic), packages/*/Makefile (helm package --version ... and fix-charts sed that writes version: 0.0.0), e.g. packages/extra/Makefile and packages/extra/bootbox/Chart.yaml.
packages/apps/kubernetes/Chart.yaml (1)
6-7: Do not quote appVersion in this PR — repo uses unquoted appVersion widelyrg found 35 Chart.yaml with unquoted appVersion (including packages/apps/kubernetes/Chart.yaml:7). Changing only this file would create inconsistency — either leave as-is or standardize appVersion quoting across all charts.
Likely an incorrect or invalid review comment.
packages/extra/ingress/Chart.yaml (1)
6-6: LGTM; ensure build overwrites the placeholder and CI guards against shipping 0.0.0.Add a pipeline check that fails if any packaged chart still has version 0.0.0.
packages/system/kubeovn-plunger/Chart.yaml (1)
5-5: LGTM; consistent with repo-wide build-time versioning.Add a CI guard to prevent publishing 0.0.0 in the chart repo index.
packages/library/cozy-lib/Chart.yaml (1)
5-5: LGTM for library chart; same CI guard applies.Library charts don’t require appVersion; placeholder version is fine if build replaces it.
packages/system/kubeovn-webhook/Chart.yaml (1)
5-5: LGTM; placeholder strategy consistent.appVersion is already set; add a CI check to block publishing version 0.0.0.
pkg/registry/apps/application/rest.go (1)
1022-1022: Confirm intent: '>= 0.0.0-0' matches all chart versions (including pre-releases)
This semver range is valid for FluxCD HelmRelease v2 and will include prerelease tags — it enables continuous automatic upgrades. Note: spec.chart.spec.version is ignored for GitRepository/Bucket sources (applies only to HelmRepository/OCI).
If you do not want automatic or prerelease upgrades, tighten the constraint or make it configurable per app/release and ensure the migrations TODO is handled before enabling auto-upgrades.
Location: pkg/registry/apps/application/rest.go:1022packages/system/Makefile (1)
11-12: Confirm the cozy- prefix for system chart names is intentional.Other Makefiles set
name: $$i(no prefix). If the prefix is required only for system charts, all good; otherwise align for consistency.packages/extra/monitoring/Chart.yaml (1)
6-6: Placeholder chart version is fine — packaging overrides it.
Confirmed Makefiles callhelm package ... --version $(COZYSTACK_VERSION)in packages/library/Makefile, packages/system/Makefile, packages/extra/Makefile, and packages/apps/Makefile.packages/apps/redis/Chart.yaml (1)
6-8: LGTM; appVersion added. Verify templates align with 6.2.6.Ensure
values.yamland templates (image tag, probes, config) are consistent with Redis 6.2.6.
| @@ -2,24 +2,6 @@ apiVersion: v2 | |||
| name: http-cache | |||
| description: Layer7 load balancer and caching service | |||
| icon: /logos/nginx.svg | |||
There was a problem hiding this comment.
🧩 Analysis chain
Icon must be an absolute URL (Helm spec); relative path likely broken after logo-copy removal.
Replace with a fully-qualified URL or drop the field to pass lint.
Apply one of these diffs:
Option A — set an absolute URL (preferred; replace with your real public URL):
-icon: /logos/nginx.svg
+icon: https://<public-base-url>/logos/nginx.svgOption B — temporarily remove to avoid invalid metadata:
-icon: /logos/nginx.svg
+# TODO: add an absolute icon URL (Helm requires a fully-qualified URL)Run this to find other charts with relative icons:
🏁 Script executed:
#!/bin/bash
rg -nP '^\s*icon:\s*/' -g '**/Chart.yaml' -C1Length of output: 4379
Helm icons must be absolute URLs — replace or remove all '/logos/…' Chart.yaml entries
Helm Chart.yaml requires a fully-qualified icon URL; relative paths (e.g., packages/apps/http-cache/Chart.yaml: icon: /logos/nginx.svg) will fail lint/packaging. Run:
rg -nP '^\sicon:\s/' -g '**/Chart.yaml' -C1
Fix each occurrence by setting a public URL or removing the field. Example:
icon: https:///logos/nginx.svg
🤖 Prompt for AI Agents
In packages/apps/http-cache/Chart.yaml around line 4, the icon field uses a
relative path ("icon: /logos/nginx.svg") which Helm rejects; change it to a
fully qualified public URL or remove the icon field. Replace the value with an
absolute URL pointing to a publicly served asset (e.g.,
https://<public-base-url>/logos/nginx.svg) or delete the icon key if no public
URL is available, then re-run lint/packaging to confirm the issue is resolved.
| # incremented each time you make changes to the application. Versions are not expected to | ||
| # follow Semantic Versioning. They should reflect the version the application is using. | ||
| # It is recommended to use it with quotes. | ||
| version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process |
There was a problem hiding this comment.
🧩 Analysis chain
Guard against publishing charts with version 0.0.0.
Using a placeholder is fine if CI always substitutes it; add a hard check to fail packaging if not replaced.
Add a CI step (or run locally) to detect placeholders:
🏁 Script executed:
#!/bin/bash
# Fail if any Chart.yaml still uses the placeholder version
set -euo pipefail
if rg -nP '^\s*version:\s*0\.0\.0\b' -g '**/Chart.yaml'; then
echo "ERROR: Placeholder chart version 0.0.0 found. Version substitution missing."; exit 1;
fiLength of output: 13387
Block packaging: fail CI when any Chart.yaml still has version: 0.0.0
rg reported many Chart.yaml files using the placeholder (including packages/apps/http-cache/Chart.yaml). Add a CI step to abort packaging/publishing if any Chart.yaml contains version: 0.0.0:
#!/bin/bash
set -euo pipefail
if rg -nP --glob '**/Chart.yaml' '^\s*version:\s*0\.0\.0\b'; then
echo "ERROR: Placeholder chart version 0.0.0 found. Version substitution missing."
exit 1
fi🤖 Prompt for AI Agents
In packages/apps/http-cache/Chart.yaml around line 6 the chart version is the
placeholder "0.0.0"; add a CI validation step (run early in packaging/publish
workflows and on PRs) that scans all Chart.yaml files for the regex matching
"version: 0.0.0" and fails the job if any are found, printing a clear error
message and exiting non‑zero; implement this as a short shell step (using rg or
grep) in your existing CI workflow (e.g., .github/workflows/* packaging/publish
jobs) so packaging is aborted when placeholder chart versions remain.
| repo: | ||
| rm -rf "$(OUT)" | ||
| helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION) | ||
| helm repo index "$(OUT)" |
There was a problem hiding this comment.
Create OUT dir before packaging; helm does not create it.
Mirror the fix across all repo targets.
repo:
rm -rf "$(OUT)"
- helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION)
- helm repo index "$(OUT)"
+ mkdir -p "$(OUT)"
+ helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION)
+ helm repo index "$(OUT)"📝 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.
| repo: | |
| rm -rf "$(OUT)" | |
| helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION) | |
| helm repo index "$(OUT)" | |
| repo: | |
| rm -rf "$(OUT)" | |
| mkdir -p "$(OUT)" | |
| helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION) | |
| helm repo index "$(OUT)" |
🤖 Prompt for AI Agents
In packages/apps/Makefile around lines 6 to 9, the recipe for the "repo" target
uses OUT but doesn't ensure the directory exists before running helm package
(helm won't create it); update the recipe to mkdir -p "$(OUT)" before calling
helm package, and apply the same mkdir -p "$(OUT)" insertion to every other
"repo" target in Makefile(s) so each ensures the OUT directory is created prior
to packaging.
| # incremented each time you make changes to the application. Versions are not expected to | ||
| # follow Semantic Versioning. They should reflect the version the application is using. | ||
| # It is recommended to use it with quotes. | ||
| version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process |
There was a problem hiding this comment.
🧩 Analysis chain
Guard against accidentally publishing charts with version 0.0.0
Using 0.0.0 as a placeholder is fine, but ensure the build always substitutes it before helm package/index. Add a CI check to fail if any packaged Chart.yaml still contains 0.0.0 to avoid repo/index pollution and upgrade issues.
Run this to verify substitution across the repo and locate packaging logic:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Charts still using placeholder versions:"
rg -nP '\bversion:\s*0\.0\.0\b' -g '**/Chart.yaml' || true
echo
echo "Packaging logic that should set chart versions:"
rg -n -C3 -g '!**/vendor/**' -e 'helm package|helm repo index|CHART_VERSION|yq|sed .*version' -e 'version.*=$(|`|\$\(.*\))' Makefile **/*.{mk,sh,py} || trueLength of output: 15220
Fail CI when any Chart.yaml still contains version: 0.0.0 and ensure the build substitutes real chart versions before packaging
rg found many Chart.yaml files still using 0.0.0 (examples: packages/apps/postgres/Chart.yaml, packages/library/cozy-lib/Chart.yaml, packages/system/victoria-metrics-operator/Chart.yaml). Packaging/search hits show scripts/package.mk references $(CHART_VERSION) but no obvious step that updates Chart.yaml was located. Add a CI gate that fails if any Chart.yaml matches '\bversion:\s*0.0.0\b' and update the build to replace placeholders before running helm package / helm repo index.
| # incremented each time you make changes to the application. Versions are not expected to | ||
| # follow Semantic Versioning. They should reflect the version the application is using. | ||
| # It is recommended to use it with quotes. | ||
| version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Guard against publishing 0.0.0 chart versions
Using 0.0.0 as a placeholder is fine, but ensure CI always overwrites it and fails if not to avoid pushing unusable charts.
Run this script to verify packaging sets chart versions and to list charts still at 0.0.0:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "helm package invocations (expect --version or pre-step that sets Chart.yaml):"
rg -n -C2 -P '\bhelm\s+package\b.*'
echo
echo "Search for explicit version setting in build scripts:"
rg -n -C2 -P '(--version\s+|CHART_VERSION|CHARTS_VERSION|chart_version\s*=|VERSION=)' -g '!**/vendor/**'
echo
echo "Charts with version: 0.0.0 (should be all placeholders only):"
rg -n -C1 -P '^\s*version:\s*0\.0\.0\b' -g '!**/vendor/**'Length of output: 1300
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Find COZYSTACK_VERSION usages/definitions:"
rg -n --hidden -S 'COZYSTACK_VERSION' || true
echo
echo "2) Show packages/apps/Makefile (confirm helm package line):"
sed -n '1,160p' packages/apps/Makefile || true
echo
echo "3) List all Chart.yaml files and their version lines:"
rg -n --hidden -S '^\s*version:\s*' -g '*/Chart.yaml' || true
echo
echo "4) List Chart.yaml files with version 0.0.0:"
rg -n --hidden -S '^\s*version:\s*0\.0\.0\b' -g '*/Chart.yaml' || true
echo
echo "5) Search CI workflows and top-level Makefiles for COZYSTACK_VERSION or explicit version-setting steps:"
rg -n --hidden -S 'COZYSTACK_VERSION|chart_version|CHARTS?_VERSION|--version\s+\$?\(|--version\s+' .github packages || true
echo
echo "6) Show top-level Makefile(s) if present (first 200 lines):"
for f in Makefile makefile GNUmakefile; do
if [ -f "$f" ]; then
echo "---- $f ----"
sed -n '1,200p' "$f"
fi
doneLength of output: 6482
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- scripts/common-envs.mk (full) ----"
sed -n '1,240p' scripts/common-envs.mk || true
echo
echo "---- .github workflow occurrences of version / tag / make repo / git fetch ----"
rg -n --hidden -S 'COZYSTACK_VERSION|git describe|git fetch|fetch --tags|GITHUB_REF|GITHUB_REF_NAME|GITHUB_SHA|make -C packages|make repos|helm package' .github || true
echo
echo "---- Chart.yaml files and their version lines (found via find) ----"
find . -type f -name Chart.yaml -print0 | while IFS= read -r -d '' f; do
printf "\n%s\n" "$f"
awk '/^\s*version:/{print; exit}' "$f" || true
done
echo
echo "---- Search for fix-charts usage ----"
rg -n --hidden -S 'fix-charts' || trueLength of output: 21038
Guard against publishing 0.0.0 chart versions
Helm packaging already passes --version $(COZYSTACK_VERSION) (packages//Makefile) and scripts/common-envs.mk derives COZYSTACK_VERSION from git tags, but many packages//Chart.yaml files are placeholders (version: 0.0.0).
- Fail fast: add an explicit check that errors when COZYSTACK_VERSION is empty or equals "0.0.0" before running helm package (add to packages/*/Makefile repo targets or scripts/common-envs.mk).
- CI: ensure workflows check out tags (actions/checkout fetch-depth: 0 or fetch-tags) so git describe --tags reliably sets COZYSTACK_VERSION.
Locations: scripts/common-envs.mk; packages//Makefile (repo target); packages//Chart.yaml.
🤖 Prompt for AI Agents
In packages/apps/vpn/Chart.yaml line 6 the Chart version is a placeholder
"0.0.0", and the repo lacks a fail-fast guard when COZYSTACK_VERSION is empty or
"0.0.0"; add a check in scripts/common-envs.mk that verifies COZYSTACK_VERSION
is set and not "0.0.0" (exit non-zero with a clear message) and add the same
guard to the packages/*/Makefile repo target before running helm package; also
update CI workflows to ensure tags are fetched (actions/checkout with
fetch-depth: 0 or fetch-tags) so git describe --tags reliably sets
COZYSTACK_VERSION.
| repo: | ||
| rm -rf "$(OUT)" | ||
| helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION) | ||
| helm repo index "$(OUT)" |
There was a problem hiding this comment.
Create OUT dir before packaging; helm does not create it.
repo:
rm -rf "$(OUT)"
- helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION)
- helm repo index "$(OUT)"
+ mkdir -p "$(OUT)"
+ helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION)
+ helm repo index "$(OUT)"📝 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.
| repo: | |
| rm -rf "$(OUT)" | |
| helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION) | |
| helm repo index "$(OUT)" | |
| repo: | |
| rm -rf "$(OUT)" | |
| mkdir -p "$(OUT)" | |
| helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION) | |
| helm repo index "$(OUT)" |
🤖 Prompt for AI Agents
In packages/extra/Makefile around lines 6 to 9, the Makefile invokes "helm
package -d $(OUT) ..." but does not ensure the OUT directory exists; add a
directory creation step (e.g., mkdir -p "$(OUT)") before the helm package
command so the target directory is created if missing. Ensure the mkdir runs
before helm package and preserves existing behavior for other commands in the
recipe.
| repo: | ||
| cd .. && ../hack/package_chart.sh library $(OUT) $(TMP) | ||
| rm -rf "$(OUT)" | ||
| helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION) | ||
| helm repo index "$(OUT)" |
There was a problem hiding this comment.
Create OUT dir before packaging; helm does not create it.
repo:
rm -rf "$(OUT)"
- helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION)
- helm repo index "$(OUT)"
+ mkdir -p "$(OUT)"
+ helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION)
+ helm repo index "$(OUT)"📝 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.
| repo: | |
| cd .. && ../hack/package_chart.sh library $(OUT) $(TMP) | |
| rm -rf "$(OUT)" | |
| helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION) | |
| helm repo index "$(OUT)" | |
| repo: | |
| rm -rf "$(OUT)" | |
| mkdir -p "$(OUT)" | |
| helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION) | |
| helm repo index "$(OUT)" |
🤖 Prompt for AI Agents
In packages/library/Makefile around lines 6 to 9, the target uses OUT but never
ensures the output directory exists which causes helm package to fail; add a
step to create the OUT directory (e.g., run mkdir -p "$(OUT)") before invoking
helm package, so the sequence becomes: remove previous OUT, recreate it with
mkdir -p "$(OUT)", run helm package into "$(OUT)", then run helm repo index
"$(OUT)".
| @@ -1,2 +1,2 @@ | |||
| name: cozy-etcd-operator | |||
| version: 0.4.0 | |||
| version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process | |||
There was a problem hiding this comment.
🧩 Analysis chain
Guard against shipping version: 0.0.0.
Add a CI check to fail if any Chart.yaml still has the placeholder at package time.
🏁 Script executed:
#!/bin/bash
# Fail if any Chart.yaml retains placeholder version
rg -nP -C1 --glob '**/Chart.yaml' '^\s*version:\s*0\.0\.0\b' || exit 0
echo "Error: placeholder version 0.0.0 found in Chart.yaml" >&2
exit 1Length of output: 21326
Add CI check to fail on Chart.yaml placeholder version 0.0.0
packages/system/etcd-operator/Chart.yaml (line 2) — and many other Chart.yaml files — still contain "version: 0.0.0". Add a CI job that fails if any Chart.yaml matches ^\sversion:\s0.0.0\b. Example check: rg -nP --glob '**/Chart.yaml' '^\sversion:\s0.0.0\b' && { echo "Error: placeholder version 0.0.0 found" >&2; exit 1; }
🤖 Prompt for AI Agents
In packages/system/etcd-operator/Chart.yaml around line 2, the file still
contains the placeholder version "0.0.0"; add a CI job that scans the repo for
any Chart.yaml with a placeholder version and fails the build if found.
Implement a CI step (e.g., in your existing pipeline YAML) that runs a
repository-wide grep/rg against the regex ^\s*version:\s*0\.0\.0\b and returns
non-zero with a clear error message when matches are found; ensure the job is
executed on PRs and main branch builds so placeholder Chart.yaml versions are
caught automatically.
| repo: | ||
| rm -rf "$(OUT)" | ||
| mkdir -p "$(OUT)" | ||
| helm package -d "$(OUT)" $$(find . -mindepth 2 -maxdepth 2 -name Chart.yaml | awk 'sub("/Chart.yaml", "")') --version $(COZYSTACK_VERSION) | ||
| helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION) | ||
| cd "$(OUT)" && helm repo index . |
There was a problem hiding this comment.
Create OUT dir before packaging; helm does not create it.
Without mkdir -p, helm package -d "$(OUT)" will fail if the directory doesn’t exist.
Apply:
repo:
rm -rf "$(OUT)"
- helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION)
- cd "$(OUT)" && helm repo index .
+ mkdir -p "$(OUT)"
+ helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION)
+ cd "$(OUT)" && helm repo index .📝 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.
| repo: | |
| rm -rf "$(OUT)" | |
| mkdir -p "$(OUT)" | |
| helm package -d "$(OUT)" $$(find . -mindepth 2 -maxdepth 2 -name Chart.yaml | awk 'sub("/Chart.yaml", "")') --version $(COZYSTACK_VERSION) | |
| helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION) | |
| cd "$(OUT)" && helm repo index . | |
| repo: | |
| rm -rf "$(OUT)" | |
| mkdir -p "$(OUT)" | |
| helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION) | |
| cd "$(OUT)" && helm repo index . |
🤖 Prompt for AI Agents
In packages/system/Makefile around lines 6 to 9, the recipe calls helm package
-d "$(OUT)" but does not ensure the OUT directory exists; add a step before
packaging to create the directory (e.g., run mkdir -p "$(OUT)") so helm has a
target directory, then proceed with helm package and the cd "$(OUT)" && helm
repo index . command.
d0092fc to
be5c68c
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/extra/seaweedfs/templates/seaweedfs.yaml (2)
12-17: Template short-circuit bug: potential render-time panic when zones is missing
len .Values.volume.zonesis evaluated even whenzonesmight be absent; Go templates don’t short-circuitand/or, so this can panic before yourfailruns. Useget/emptyand computelenonly after existence is ensured.Apply this diff:
-{{- if (eq (len .Values.volume.zones) 0) }} -{{- fail "Zones must be defined for MultiZone topology." }} -{{- end }} -{{- if and (hasKey .Values.volume "zones") (gt (int .Values.replicationFactor) (len .Values.volume.zones)) }} -{{- fail "replicationFactor must be less than or equal to the number of zones defined in .Values.volume.zones." }} -{{- end }} +{{- $zones := (get .Values.volume "zones") }} +{{- if empty $zones }} +{{- fail "Zones must be defined for MultiZone topology." }} +{{- end }} +{{- if gt (int .Values.replicationFactor) (len $zones) }} +{{- fail "replicationFactor must be less than or equal to the number of zones defined in .Values.volume.zones." }} +{{- end }}
109-110: Wrong fallback path: should use .Values.volume.sizeThe fallback references
$.Values.size, likely undefined. It should reference$.Values.volume.size.- {{- else }} - size: "{{ $.Values.size }}" + {{- else }} + size: "{{ $.Values.volume.size }}"
🧹 Nitpick comments (18)
packages/extra/ingress/templates/nginx-ingress.yaml (1)
24-24: Reconsider forcing upgrades globally; risk of downtime and LB IP churn.
upgrade.force: truemay delete/recreate immutable resources (e.g., Service), causing brief outages and possibly changing LoadBalancer IPs. Given the broad version constraint (Line 17) this can amplify unintended rollouts.Recommend making force opt-in via values and defaulting to false.
Apply this minimal change to parameterize force:
upgrade: - force: true + force: {{ default false .Values.forceUpgrade }}Operationally, only enable
forceUpgradewhen addressing specific immutable-field changes; otherwise keep it disabled to reduce blast radius.packages/apps/kubernetes/templates/helmreleases/volumesnapshot_crd.yaml (1)
18-18: Consider narrowing the chart version range.
'>= 0.0.0-0'will drift to any future release, which is risky for CRDs. Recommend pinning to a compatible major/minor range (e.g.,^1.5.0or>=1.5.0 <2.0.0) and advancing intentionally.packages/extra/seaweedfs/templates/seaweedfs.yaml (3)
37-39: Guard against missing namespace annotations
index $myNS.metadata.annotations ...errors if the annotation is absent. Usegetwith a safe default.-{{- $ingress := index $myNS.metadata.annotations "namespace.cozystack.io/ingress" }} -{{- $host := index $myNS.metadata.annotations "namespace.cozystack.io/host" }} +{{- $annotations := (get $myNS.metadata "annotations") | default (dict) }} +{{- $ingress := (get $annotations "namespace.cozystack.io/ingress") | default "" }} +{{- $host := (get $annotations "namespace.cozystack.io/host") | default "" }}
57-62: Unbounded remediation retries can cause churn
retries: -1for both install/upgrade may loop indefinitely during persistent failures.Consider finite retries with backoff or adding alerting around repeated failures.
117-119: nodeSelector should be a map, not a block string (verify chart expectations)Using
|creates a string, not a map. Most charts expect a YAML map fornodeSelector.If the seaweedfs chart expects a map, use:
- nodeSelector: | - topology.kubernetes.io/zone: {{ $zoneName }} + nodeSelector: + topology.kubernetes.io/zone: {{ $zoneName }}If the chart expects raw YAML in a string, ignore this.
packages/apps/tenant/templates/seaweedfs.yaml (1)
26-28: upgrade.force: true can be disruptive; add safeguards or justify necessity.--force may delete/recreate resources and, paired with infinite retries, can cause repeated disruptive cycles on persistent workloads like SeaweedFS. If you truly need force, consider adding rollback/cleanup safeguards.
Apply:
upgrade: - force: true - remediation: - retries: -1 + force: true + cleanupOnFail: true + remediation: + retries: -1 + remediateLastFailure: true + strategy: rollbackAlternatively, drop force unless there is a known immutable‑field change that requires it.
packages/apps/kubernetes/templates/helmreleases/victoria-metrics-operator.yaml (1)
33-33: Re-evaluateupgrade.force: truefor the operator.Force can recreate resources on immutable changes, causing brief disruption. If not strictly needed, prefer omitting it.
Apply this diff to drop force:
upgrade: - force: true remediation: retries: -1packages/apps/tenant/templates/monitoring.yaml (1)
26-26: Force upgrade likely unnecessary here.Unless you’re routinely hitting immutable field changes, prefer non-forced upgrades to reduce churn.
upgrade: - force: true remediation: retries: -1packages/apps/kubernetes/templates/helmreleases/velero.yaml (2)
26-27: Consider a longer timeout for Velero.Initial installs and upgrades can exceed 10m in some environments. Suggest 20m.
- timeout: 10m + timeout: 20mHas 10m proven sufficient on clusters with larger object counts or slow registries?
33-33: Caution withupgrade.forceon backup components.Forced replacement may restart deployments/daemonsets, briefly impacting backup/restore operations. Recommend dropping unless needed for immutable-field migrations.
upgrade: - force: true remediation: retries: -1packages/core/platform/templates/apps.yaml (1)
4-5: Remove duplicate$hostinitialization.Two consecutive assignments to
$hostare redundant.-{{- $host := "example.org" }} -{{- $host := "example.org" }} +{{- $host := "example.org" }}packages/apps/kubernetes/templates/helmreleases/cert-manager.yaml (1)
33-33: Consider avoidingupgrade.forceunless required.It may trigger resource re-creation on immutable changes; usually unnecessary for cert-manager components when CRDs are managed separately.
upgrade: - force: true remediation: retries: -1packages/apps/kubernetes/templates/helmreleases/gpu-operator.yaml (1)
26-27: Increase timeout for GPU Operator.Install/upgrade often takes longer (driver/toolkit, node init). Recommend 30m.
- timeout: 10m + timeout: 30mpackages/apps/kubernetes/templates/helmreleases/ingress-nginx.yaml (1)
45-54: Gate upgrade.force behind a value to avoid unintended disruptive rollouts.Helm --force can replace resources and cause downtime. Recommend making this configurable and defaulting to false.
- upgrade: - force: true + upgrade: + force: {{ default false .Values.addons.ingressNginx.forceUpgrade }} remediation: retries: -1Consider a global switch (e.g., .Values.global.forceUpgrade) if you want a uniform control.
packages/apps/nats/templates/nats.yaml (1)
39-47: Be cautious with upgrade.force for NATS.Forced upgrades may re-create resources and disrupt the cluster. Consider a value-gated toggle and ensure proper readiness/PodDisruptionBudget to avoid message loss.
- upgrade: - force: true + upgrade: + force: {{ default false .Values.forceUpgrade }} remediation: retries: -1Confirm you have PDBs and safe rollout strategy for NATS to tolerate forced upgrades.
packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml (1)
61-61: Reconsider upgrade.force for VPA (can be disruptive).force may recreate resources; for control-plane components like VPA this can cause avoidable restarts. Consider dropping it or making it conditional per chart.
packages/apps/kubernetes/templates/helmreleases/coredns.yaml (1)
37-37: Avoid upgrade.force for CoreDNS unless strictly needed.CoreDNS is critical-path; force can recreate resources and briefly disrupt DNS. Prefer leaving it unset unless you have a concrete incompatibility to resolve.
pkg/registry/apps/application/rest.go (1)
1031-1041: Add Timeout to match templates and avoid long-hanging reconciles.Parity with templates and better failure bounds.
Apply this diff:
Spec: helmv2.HelmReleaseSpec{ @@ - Interval: metav1.Duration{Duration: 5 * time.Minute}, + Interval: metav1.Duration{Duration: 5 * time.Minute}, + Timeout: metav1.Duration{Duration: 10 * time.Minute}, Install: &helmv2.Install{ Remediation: &helmv2.InstallRemediation{ Retries: -1, }, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/apps/bucket/templates/helmrelease.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/cert-manager-crds.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/cert-manager.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/cilium.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/coredns.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/fluxcd.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/gateway-api-crds.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/gpu-operator.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/ingress-nginx.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/monitoring-agents.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/velero.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/victoria-metrics-operator.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/volumesnapshot_crd.yaml(1 hunks)packages/apps/nats/templates/nats.yaml(1 hunks)packages/apps/tenant/templates/etcd.yaml(1 hunks)packages/apps/tenant/templates/info.yaml(1 hunks)packages/apps/tenant/templates/ingress.yaml(1 hunks)packages/apps/tenant/templates/monitoring.yaml(1 hunks)packages/apps/tenant/templates/seaweedfs.yaml(1 hunks)packages/core/platform/templates/apps.yaml(1 hunks)packages/extra/ingress/templates/nginx-ingress.yaml(1 hunks)packages/extra/seaweedfs/templates/seaweedfs.yaml(1 hunks)pkg/registry/apps/application/rest.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (20)
packages/extra/ingress/templates/nginx-ingress.yaml (1)
18-19: Interval/timeout additions look good.Values are reasonable. Ensure consistency with other HelmReleases in this repo so reconcile cadence and action timeouts are uniform.
packages/apps/kubernetes/templates/helmreleases/volumesnapshot_crd.yaml (2)
25-26: LGTM: interval/timeout placement under spec is correct.This matches HelmRelease (Flux) spec; good move centralizing reconciliation timing here.
1-1: Confirm HelmRelease v2 API availability in your clusters.apiVersion
helm.toolkit.fluxcd.io/v2requires helm-controller with v2 APIs. If any target cluster runs an older controller (v2beta2), this manifest will fail to apply.packages/extra/seaweedfs/templates/seaweedfs.yaml (2)
54-55: Duration formats: confirm Flux HelmRelease accepts unquoted durations
interval: 5mandtimeout: 10mlook correct for helm.toolkit.fluxcd.io/v2. Some linters prefer quoting durations for clarity.Would you like me to scan the repo for inconsistent duration formats and align them?
168-245: WorkloadMonitor version uses Chart.Version (now 0.0.0 placeholder)With placeholder versions, these fields may become
0.0.0. If any automation or UI relies on this for drift/visibility, confirm the build replaces it or switch to appVersion.I can scan other charts to ensure consistent use of
.Chart.AppVersionvs.Chart.Versionand propose a batch fix if desired.packages/apps/tenant/templates/seaweedfs.yaml (1)
20-21: Interval raised to 5m — confirm SLO alignment and consistency.5m/10m is valid and matches the majority of HelmReleases (including packages/apps/tenant/templates/seaweedfs.yaml); exceptions: packages/system/bootbox/templates/bootbox.yaml uses interval: 1m0s (timeout: 5m0s) and packages/core/platform/templates/apps.yaml uses interval: 0s. Confirm the increased reconcile latency vs 1m is intentional for rollouts/recovery or standardize intervals across HelmReleases.
packages/apps/kubernetes/templates/helmreleases/victoria-metrics-operator.yaml (1)
26-27: LGTM on reconcile timing.Moving interval and adding timeout under spec is correct for HelmRelease v2.
Please confirm helm-controller in target clusters is >= v0.34 (supports spec.timeout on HelmRelease v2).
packages/apps/kubernetes/templates/helmreleases/cilium.yaml (1)
40-41: OK on interval/timeout placement.Spec-level interval and timeout look good.
packages/apps/kubernetes/templates/helmreleases/cert-manager-crds.yaml (1)
26-27: Interval/timeout placement is fine.packages/apps/tenant/templates/monitoring.yaml (1)
20-21: Good: standardized interval/timeout.Spec-level interval/timeout looks consistent with the repo-wide pattern.
packages/core/platform/templates/apps.yaml (1)
51-51: LGTM on chart version constraint.Using
'>= 0.0.0-0'aligns with the new build-time versioning and picks latest including pre-releases.Ensure helm-controller semantics meet your expectations: this will select the highest published version (incl. pre-releases).
packages/apps/kubernetes/templates/helmreleases/cert-manager.yaml (1)
26-27: OK on reconcile timing.packages/apps/kubernetes/templates/helmreleases/monitoring-agents.yaml (1)
28-37: Good standardization on interval/timeout and remediation.The 5m/10m timings with remediation and forced upgrades align with the repo-wide pattern.
If keeping upgrade.force globally, confirm your SLOs tolerate forced re‑creates for monitoring agents during upgrades.
packages/apps/bucket/templates/helmrelease.yaml (1)
15-23: LGTM; standardized timings and remediation are clear.Spec-level interval/timeout and remediation blocks look correct.
packages/apps/tenant/templates/ingress.yaml (1)
20-29: Nice: explicit version range, timings, and remediation.This aligns with the new packaging strategy and resiliency posture.
packages/apps/kubernetes/templates/helmreleases/gateway-api-crds.yaml (1)
26-34: Approve — -1 allowed for unlimited retries in HelmRelease v2Interval/timeout and upgrade.force mirror other templates; HelmRelease v2 accepts negative integers (e.g. -1) for install/upgrade remediation.retries to indicate unlimited retries.
packages/apps/kubernetes/templates/helmreleases/fluxcd.yaml (1)
30-35: Resolved — remediation.retries = -1 is valid for HelmRelease v2Flux helm-controller and the HelmRelease CRD treat a negative integer (e.g. -1) as “unlimited” for both spec.install.remediation.retries and spec.upgrade.remediation.retries, so this will not cause CRD validation errors.
packages/apps/kubernetes/templates/helmreleases/coredns.yaml (1)
31-32: Spec.interval/timeout LGTM.Placement and values look good.
pkg/registry/apps/application/rest.go (1)
1021-1023: Switch to wildcard chart constraint: confirm this is intentional/backward-compatible.Using ">= 0.0.0-0" ignores any app-level versioning. Ensure clients don’t rely on pinning and that this won’t pull unintended major upgrades.
packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml (1)
54-55: Confirmed: HelmRelease v2 supports spec.timeout and "10m" is valid
spec.timeout is metav1.Duration and accepts Go duration strings (e.g. "10m" = 10 minutes).
| remediation: | ||
| retries: -1 | ||
| upgrade: | ||
| force: true |
There was a problem hiding this comment.
🧩 Analysis chain
Do not force-upgrade CRDs.
If this chart templates CRDs (not under crds/), upgrade.force: true may delete/recreate CRDs on immutable changes, risking loss of all custom resources. Strongly recommend removing.
upgrade:
- force: true
remediation:
retries: -1Please confirm how cozy-cert-manager-crds packages CRDs (templates vs crds/). If under crds/, Helm won’t upgrade them anyway and force is ineffective; if templated, force is dangerous.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Show target file =="
sed -n '1,240p' packages/apps/kubernetes/templates/helmreleases/cert-manager-crds.yaml || true
echo
echo "== Ripgrep for cert-manager / cozy occurrences =="
rg -n --hidden -S "cozy-cert-manager|cert-manager-crds|cert-manager-crd|certmanager" || true
echo
echo "== Ripgrep for 'upgrade' + 'force' occurrences =="
rg -n --hidden -U -S "upgrade:\s*\n(?:\s+.+\n)*\s*force:\s*true" || true
rg -n --hidden -S "force:\s*true" || true
echo
echo "== Find crds directories =="
find . -type d -name crds -print || true
echo
echo "== List charts under packages/apps/kubernetes (if any) =="
ls -la packages/apps/kubernetes || true
ls -la packages/apps/kubernetes/charts || true
echo
echo "== Search for Chart.yaml entries mentioning cert-manager =="
rg -n --hidden -S "cert-manager|certmanager" Chart.yaml || trueLength of output: 57396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Chart root =="
ls -la packages/system/cert-manager-crds || true
echo
echo "== Find crds dir under chart =="
find packages/system/cert-manager-crds -maxdepth 3 -type d -name crds -print || true
echo
echo "== List files under chart (top-level) =="
find packages/system/cert-manager-crds -maxdepth 2 -type f -print -exec sed -n '1,120p' {} \; -exec echo "----" \; || true
echo
echo "== Search for CRD markers in chart =="
rg -n --hidden -S "kind:\s*CustomResourceDefinition|apiextensions.k8s.io/v1" packages/system/cert-manager-crds || true
echo
echo "== Chart.yaml =="
sed -n '1,200p' packages/system/cert-manager-crds/Chart.yaml || true
echo
echo "== templates dir listing =="
ls -la packages/system/cert-manager-crds/templates || true
echoLength of output: 2866
Remove force-upgrade for cert-manager CRDs.
cozy-cert-manager-crds ships CRDs as templated resources (packages/system/cert-manager-crds/charts/cert-manager/templates/crds.yaml); upgrade.force: true passes --force and can delete/recreate CRDs on immutable changes, risking loss of all CustomResources — remove it from packages/apps/kubernetes/templates/helmreleases/cert-manager-crds.yaml.
upgrade:
- force: true
remediation:
retries: -1📝 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.
| force: true | |
| upgrade: | |
| remediation: | |
| retries: -1 |
🤖 Prompt for AI Agents
In packages/apps/kubernetes/templates/helmreleases/cert-manager-crds.yaml around
line 33, the HelmRelease sets upgrade.force: true which causes helm to pass
--force on CRD updates (risking deletion/recreation of CRDs and loss of
CustomResources); remove the force: true entry (or set upgrade.force to
false/omit the field entirely) so CRDs are not force-upgraded.
| remediation: | ||
| retries: -1 | ||
| upgrade: | ||
| force: true |
There was a problem hiding this comment.
❓ Verification inconclusive
Avoid upgrade.force: true on CNI (risk of dataplane disruption).
Helm --force can delete/recreate DaemonSets and related objects, potentially disrupting networking. Recommend dropping unless you’ve validated safe rolling behavior for your environment.
upgrade:
- force: true
remediation:
retries: -1If you intend to keep force, please confirm:
- Cilium upgrade plan tested on representative clusters
- Drain/cordon or surge strategy in place to avoid control-plane dataplane loss
- Adequate PDBs and readiness gates
Remove upgrade.force: true from Cilium HelmRelease
Helm --force can delete/recreate DaemonSets and disrupt CNI dataplane; remove unless upgrades are validated.
Location: packages/apps/kubernetes/templates/helmreleases/cilium.yaml:47
upgrade:
- force: true
remediation:
retries: -1If you intend to keep force, confirm:
- Cilium upgrade plan tested on representative clusters
- Drain/cordon or surge strategy in place to avoid dataplane loss
- Adequate PDBs and readiness gates
📝 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.
| force: true | |
| upgrade: | |
| remediation: | |
| retries: -1 |
🤖 Prompt for AI Agents
In packages/apps/kubernetes/templates/helmreleases/cilium.yaml around line 47,
the HelmRelease sets upgrade.force: true which can delete/recreate DaemonSets
and disrupt the CNI dataplane; remove the force: true setting (or set it to
false) so upgrades use normal Helm upgrade behavior. If you must keep force,
instead add a clear comment and validation steps and ensure cluster update
strategy (drain/cordon or surge), proper PDBs and readiness gates are configured
before re-enabling force.
| remediation: | ||
| retries: -1 | ||
| upgrade: | ||
| force: true |
There was a problem hiding this comment.
🧩 Analysis chain
Be careful with upgrade.force on GPU Operator.
Forced upgrades can recreate operator/daemonsets, disrupting GPU workloads. Prefer standard upgrades unless remediating immutable fields.
upgrade:
- force: true
remediation:
retries: -1If keeping force, confirm you have cordon/drain and workload disruption windows defined.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "Searching for gpu-operator and matching HelmRelease files..."
rg -n --hidden --no-ignore 'gpu-operator|helmreleases|upgrade:' || true
echo
file="packages/apps/kubernetes/templates/helmreleases/gpu-operator.yaml"
if [ -f "$file" ]; then
echo "=== Showing $file ==="
sed -n '1,160p' "$file"
else
echo "$file not found — listing matching files containing 'helmreleases' or 'upgrade'"
rg -n --hidden --no-ignore 'helmreleases' || true
rg -n --hidden --no-ignore 'upgrade:' || true
fiLength of output: 18579
Remove upgrade.force: true from GPU Operator HelmRelease
Forced upgrades can recreate the operator/daemonsets and disrupt GPU workloads; use standard upgrades unless you must remediate immutable fields. If keeping force, confirm cordon/drain and an explicit disruption window are in place.
Location: packages/apps/kubernetes/templates/helmreleases/gpu-operator.yaml (upgrade block)
upgrade:
- force: true
remediation:
retries: -1📝 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.
| force: true | |
| upgrade: | |
| remediation: | |
| retries: -1 |
🤖 Prompt for AI Agents
In packages/apps/kubernetes/templates/helmreleases/gpu-operator.yaml around line
33, remove the upgrade.force: true setting (or set it to false) to avoid forced
upgrades; if you must retain force: true for a specific immutable-field
remediation, add a clear comment documenting the reason and ensure cluster
maintenance steps are configured (cordon/drain nodes and an explicit disruption
window) before enabling it.
| install: | ||
| createNamespace: true | ||
| remediation: | ||
| retries: -1 | ||
| upgrade: | ||
| force: true | ||
| remediation: | ||
| retries: -1 |
There was a problem hiding this comment.
Avoid upgrade.force: true for CRD-only charts; use CRD policies instead.
Force upgrades can delete/recreate resources and risk CRD/CR data disruption. Prefer explicit CRD handling via install/upgrade CRD policies.
Apply:
install:
createNamespace: true
remediation:
retries: -1
+ crds: CreateReplace
upgrade:
- force: true
+ crds: CreateReplace
remediation:
retries: -1Notes:
- CreateReplace updates CRDs safely without blanket force on all resources.
- If CRDs are shipped via chart’s crds/ directory (Helm semantics), this aligns with Flux v2 CRD policy handling.
📝 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.
| install: | |
| createNamespace: true | |
| remediation: | |
| retries: -1 | |
| upgrade: | |
| force: true | |
| remediation: | |
| retries: -1 | |
| install: | |
| createNamespace: true | |
| remediation: | |
| retries: -1 | |
| crds: CreateReplace | |
| upgrade: | |
| crds: CreateReplace | |
| remediation: | |
| retries: -1 |
🤖 Prompt for AI Agents
In packages/apps/kubernetes/templates/helmreleases/volumesnapshot_crd.yaml
around lines 27 to 34, the HelmRelease currently sets upgrade.force: true which
can delete/recreate resources and risk CRD/CR data; remove upgrade.force: true
and instead configure explicit CRD handling by adding CRD create/upgrade
policies (e.g., set install.crds.create and upgrade.crds.create to CreateReplace
or the equivalent Flux CRD policy) so CRDs are updated safely without forcing
all resources.
| interval: 5m | ||
| timeout: 10m | ||
| install: | ||
| remediation: | ||
| retries: -1 | ||
| upgrade: | ||
| force: true | ||
| remediation: | ||
| retries: -1 |
There was a problem hiding this comment.
Do not set upgrade.force for etcd (risk of disruption/data loss).
Force re-creates resources during upgrade. For stateful etcd this is high-risk. Recommend removing force here.
Apply this diff:
upgrade:
- force: true
remediation:
retries: -1📝 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.
| interval: 5m | |
| timeout: 10m | |
| install: | |
| remediation: | |
| retries: -1 | |
| upgrade: | |
| force: true | |
| remediation: | |
| retries: -1 | |
| interval: 5m | |
| timeout: 10m | |
| install: | |
| remediation: | |
| retries: -1 | |
| upgrade: | |
| remediation: | |
| retries: -1 |
🤖 Prompt for AI Agents
packages/apps/tenant/templates/etcd.yaml around lines 20 to 28: the chart sets
upgrade.force: true which can re-create resources and is dangerous for stateful
etcd; remove the "force: true" line under the upgrade block (or set it to false)
so upgrades do not force-recreate resources, leaving the remediation retries
as-is.
| interval: 5m | ||
| timeout: 10m | ||
| install: | ||
| remediation: | ||
| retries: -1 | ||
| upgrade: | ||
| force: true | ||
| remediation: | ||
| retries: -1 |
There was a problem hiding this comment.
Add a chart version constraint for consistency and safety.
Other releases pin with version: '>= 0.0.0-0'. This one lacks a version, which can lead to unexpected chart upgrades.
Apply this diff:
sourceRef:
kind: HelmRepository
name: cozystack-extra
namespace: cozy-public
+ chart:
+ spec:
+ version: '>= 0.0.0-0'If you prefer not to re-indent blocks, alternatively add version under the existing chart.spec.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/apps/tenant/templates/info.yaml around lines 22 to 30, this release
is missing a chart version constraint; add a version field set to '>= 0.0.0-0'
under the chart spec (or directly under chart if you prefer not to re-indent) so
the file matches other releases and prevents unexpected upgrades.
| remediation: | ||
| retries: -1 | ||
| upgrade: | ||
| force: true |
There was a problem hiding this comment.
Upgrade with force can be disruptive
upgrade.force: true may replace resources on immutable-field changes, potentially causing downtime. Make it opt‑in via values.
- upgrade:
- force: true
+ upgrade:
+ force: {{ .Values.upgrade.force | default false }}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/extra/seaweedfs/templates/seaweedfs.yaml around line 60, the chart
hardcodes "force: true" for upgrades which can be disruptive; make this behavior
opt‑in by replacing the hardcoded value with a templated value tied to
values.yaml (e.g. use an upgrade.force value defaulting to false), update
values.yaml to add upgrade.force: false with a comment explaining the risk, and
ensure any README/notes mention that enabling upgrade.force is optional and may
cause resource replacement/downtime.
be5c68c to
cc36d17
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/apps/Makefile (3)
2-2: Use deferred assignment for CHARTS to reduce staleness.Makes CHARTS recompute on use (useful when adding charts in the same invocation).
Apply this diff:
-CHARTS := $(shell find . -maxdepth 2 -name Chart.yaml | awk -F/ '{print $$2}') +CHARTS = $(shell find . -maxdepth 2 -name Chart.yaml | awk -F/ '{print $$2}')
11-12: Prefer a YAML-aware tool over sed; improve portability.sed -i differs on macOS; and regex assumes top-level keys without leading spaces. Consider yq for safety, or make sed more robust.
Option A (recommended, requires yq):
find . -maxdepth 2 -name Chart.yaml -print0 | xargs -0 -n1 sh -c 'd=$$(dirname "$$1"); n=$$(basename "$$d"); yq -i ".name = \"$$n\" | .version = \"0.0.0\"" "$$1"' _Option B (keep sed; handle whitespace; GNU sed assumed):
find . -maxdepth 2 -name Chart.yaml | awk -F/ '{print $$2}' | while read i; do \ sed -i -E -e "s/^name:\s*.*/name: $$i/" \ -e "s/^version:\s*.*/version: 0.0.0 # Placeholder, actual version set during build/" "$$i/Chart.yaml"; \ donePlease confirm GNU sed is available in your build environment.
1-12: Add phony/default/clean/test targets to satisfy checkmake and improve UX.Makes targets explicit and fixes minphony warnings.
Add these lines (outside the shown ranges):
.PHONY: all repo fix-charts clean test all: repo clean: @rm -rf "$(OUT)" test: @echo "No tests for packages/apps"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
packages/apps/Makefile(1 hunks)packages/apps/bucket/templates/helmrelease.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/cert-manager-crds.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/cert-manager.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/cilium.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/coredns.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/fluxcd.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/gateway-api-crds.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/gpu-operator.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/ingress-nginx.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/monitoring-agents.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/velero.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/victoria-metrics-operator.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/volumesnapshot_crd.yaml(1 hunks)packages/apps/nats/templates/nats.yaml(1 hunks)packages/apps/tenant/templates/etcd.yaml(1 hunks)packages/apps/tenant/templates/info.yaml(1 hunks)packages/apps/tenant/templates/ingress.yaml(1 hunks)packages/apps/tenant/templates/monitoring.yaml(1 hunks)packages/apps/tenant/templates/seaweedfs.yaml(1 hunks)packages/core/platform/templates/apps.yaml(1 hunks)packages/extra/ingress/templates/nginx-ingress.yaml(1 hunks)packages/extra/seaweedfs/templates/seaweedfs.yaml(1 hunks)pkg/registry/apps/application/rest.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/apps/kubernetes/templates/helmreleases/victoria-metrics-operator.yaml
- packages/apps/kubernetes/templates/helmreleases/cilium.yaml
- packages/apps/nats/templates/nats.yaml
- packages/apps/tenant/templates/monitoring.yaml
- packages/apps/tenant/templates/seaweedfs.yaml
- packages/apps/kubernetes/templates/helmreleases/gateway-api-crds.yaml
- packages/apps/kubernetes/templates/helmreleases/cert-manager-crds.yaml
- packages/apps/kubernetes/templates/helmreleases/coredns.yaml
- packages/apps/tenant/templates/etcd.yaml
- packages/extra/seaweedfs/templates/seaweedfs.yaml
- pkg/registry/apps/application/rest.go
- packages/extra/ingress/templates/nginx-ingress.yaml
- packages/apps/kubernetes/templates/helmreleases/gpu-operator.yaml
- packages/apps/kubernetes/templates/helmreleases/cert-manager.yaml
- packages/apps/kubernetes/templates/helmreleases/velero.yaml
- packages/apps/bucket/templates/helmrelease.yaml
- packages/apps/kubernetes/templates/helmreleases/volumesnapshot_crd.yaml
- packages/apps/kubernetes/templates/helmreleases/ingress-nginx.yaml
- packages/apps/kubernetes/templates/helmreleases/monitoring-agents.yaml
- packages/core/platform/templates/apps.yaml
🧰 Additional context used
🪛 checkmake (0.2.2)
packages/apps/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (12)
packages/apps/tenant/templates/info.yaml (2)
14-21: Add a chart version constraint to avoid unexpected upgradesThis HelmRelease lacks spec.chart.spec.version (contrast with ingress.yaml). Add '>= 0.0.0-0' for consistency and safety.
sourceRef: kind: HelmRepository name: cozystack-extra namespace: cozy-public + version: '>= 0.0.0-0'
22-30: Confirm unlimited remediation retries are intentionalinterval/timeout changes look fine. Please confirm install/upgrade remediation.retries = -1 (infinite) is desired; combined with upgrade.force this can loop on a broken chart until fixed.
packages/apps/tenant/templates/ingress.yaml (1)
20-29: LGTM: version constraint added; verify infinite retries choice
- Good: version: '>= 0.0.0-0' under chart.spec matches the new pattern.
- interval/timeout and remediation settings align with the PR’s direction. As above, please confirm retries: -1 is intentional with upgrade.force enabled.
packages/apps/kubernetes/templates/helmreleases/fluxcd.yaml (4)
26-27: Good addition: interval and timeout for the operator HelmRelease.Spec placement and values look correct.
33-35: Good: enable forced upgrades for the operator.Matches the pattern used elsewhere in this PR.
57-79: Add parity: timeout and upgrade.force for the second HelmRelease.For consistent behavior and remediation semantics, mirror timeout and force on the non-operator release.
Apply:
spec: interval: 5m + timeout: 10m releaseName: fluxcd @@ upgrade: + force: true remediation: retries: -1
30-35: Confirmed — remediation.retries = -1 is supported as unlimited.Flux/helm-controller treats negative integers (e.g. -1) as infinite retries; no change needed.
packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml (2)
54-55: LGTM: interval and timeout added.Matches the standardized pattern in this PR.
61-63: LGTM: upgrade.force enabled.Consistent with other HelmReleases here.
packages/apps/Makefile (3)
1-1: OUT path fix LGTM.Now points to the repo-root _out, consistent with other packages.
6-9: Confirm intent to wipe OUT before indexing; consider merging existing index.If you need to preserve previously published packages/metadata locally, prefer merging:
You can replace the clean-slate approach with:
# keep previously packaged charts and merge index helm repo index "$(OUT)" --merge "$(OUT)/index.yaml"
6-9: Create OUT dir and validate inputs before Helm packaging.Without mkdir -p, helm package fails after rm -rf. Also guard empty COZYSTACK_VERSION/CHARTS and quote the flag.
Apply this diff:
repo: - rm -rf "$(OUT)" - helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION) - helm repo index "$(OUT)" + rm -rf "$(OUT)" + mkdir -p "$(OUT)" + : "$(COZYSTACK_VERSION)" # fail if empty + test -n "$(CHARTS)" || { echo "No charts found under $$PWD"; exit 1; } + helm package -d "$(OUT)" $(CHARTS) --version "$(COZYSTACK_VERSION)" + helm repo index "$(OUT)"
cc36d17 to
f0cf7b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (14)
packages/apps/kubernetes/templates/helmreleases/coredns.yaml (1)
37-37: Be cautious with upgrade.force: true on a core componentForce can delete/recreate resources if a patch fails, risking brief DNS disruption. Consider making this opt-out/opt-in via values to control blast radius per environment.
Apply this diff to make behavior configurable while keeping current default:
- force: true + force: {{ .Values.addons.coredns.forceUpgrade | default true }}Optionally set .Values.addons.coredns.forceUpgrade=false in sensitive clusters.
packages/apps/kubernetes/templates/helmreleases/cert-manager.yaml (1)
33-33: Scope upgrade.force to reduce unintended restartsFor controllers like cert-manager, force may recreate resources during incompatible changes, causing downtime if replicas/PDB aren’t sufficient. Make it configurable per environment.
Apply this diff:
- force: true + force: {{ .Values.addons.certManager.forceUpgrade | default true }}If HA isn’t guaranteed, consider defaulting to false and enabling only when needed.
packages/apps/nats/templates/nats.yaml (3)
41-44: Unlimited install retries can mask misconfig; make configurable or cap
retries: -1retries forever. Suggest parameterizing or bounding to avoid silent infinite loops and to surface issues sooner.- install: - remediation: - retries: -1 + install: + remediation: + retries: {{ .Values.helm.installRemediationRetries | default 5 }}Optionally add alerts around repeated failures.
45-45: Force upgrades risk delete/recreate → potential NATS downtime
upgrade.force: truecan trigger delete/recreate on immutable changes (e.g., StatefulSets), causing disruption. Recommend gating via a value and defaulting to false.- force: true + force: {{ .Values.helm.upgradeForce | default false }}
46-47: Upgrade remediation: consider explicit rollback strategy and bounded retriesUnlimited retries persist failures indefinitely. Make behavior explicit and configurable; prefer rollback over uninstall for safety.
- remediation: - retries: -1 + remediation: + retries: {{ .Values.helm.upgradeRemediationRetries | default 5 }} + remediateLastFailure: true + strategy: rollbackpackages/apps/tenant/templates/monitoring.yaml (2)
20-20: Confirm intent to include prereleases; otherwise tighten the range.
'>= 0.0.0-0'matches all prereleases (e.g., rc, beta). If you don’t want Flux to auto-adopt prereleases, use'>= 0.0.0'or a bounded major/minor range.Apply if prereleases should be excluded:
- version: '>= 0.0.0-0' + version: '>= 0.0.0'
27-27:upgrade.force: truecan cause disruptive recreations—consider gating and safer defaults.Force replaces immutable resources and may cause downtime. Recommend making it configurable and enabling atomic rollbacks.
Proposed tweak:
upgrade: - force: true + force: {{ .Values.upgrade.force | default false }} + atomic: true + cleanupOnFail: trueIf you intend to force by default, confirm this is acceptable for monitoring SLOs and any in‑chart migrations.
packages/apps/Makefile (3)
2-2: Make CHARTS discovery more robust (avoid awk split assumptions).This version tolerates paths consistently and extracts basenames directly.
-CHARTS := $(shell find . -maxdepth 2 -name Chart.yaml | awk -F/ '{print $$2}') +CHARTS := $(shell find . -mindepth 2 -maxdepth 2 -type f -name Chart.yaml -exec dirname {} \; | xargs -n1 basename)
11-12: sed -i is not portable to macOS; consider backup-inplace for cross‑platform.BSD sed requires an extension argument; using .bak keeps compatibility.
-fix-charts: - find . -maxdepth 2 -name Chart.yaml | awk -F/ '{print $$2}' | while read i; do sed -i -e "s/^name: .*/name: $$i/" -e "s/^version: .*/version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process/g" "$$i/Chart.yaml"; done +fix-charts: + find . -maxdepth 2 -name Chart.yaml | awk -F/ '{print $$2}' | while read i; do \ + sed -i.bak -e "s/^name: .*/name: $$i/" \ + -e "s/^version: .*/version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process/g" "$$i/Chart.yaml"; \ + rm -f "$$i/Chart.yaml.bak"; \ + done
1-12: Add standard phony targets to satisfy checkmake and improve DX.Add .PHONY and an all/clean/test scaffold.
Add the following (outside current ranges, at file top or bottom):
.PHONY: all repo fix-charts clean test all: repo clean: rm -rf -- "$(OUT)" test: helm lint $(CHARTS)packages/apps/tenant/templates/ingress.yaml (2)
20-20: Avoid unbounded chart upgrades; make the chart version configurable or pin a safe range.
'>= 0.0.0-0'will track any release (including pre-releases), risking surprise upgrades. Prefer pinning to a major/minor range or making it Values-driven so operators can control rollout.Apply this diff:
- version: '>= 0.0.0-0' + version: {{ .Values.charts.ingress.version | default ">= 0.0.0-0" | quote }}
23-25: Limit remediation retries; enable rollback & remediateLastFailure (HelmRelease v2 supports these fields)HelmRelease v2 accepts
retries: -1(unlimited) and supportsupgrade.strategy: rollbackandremediateLastFailure; unlimited retries can cause churn — prefer bounded retries +remediateLastFailure.File: packages/apps/tenant/templates/ingress.yaml lines 23-25 and 28-29
Apply this diff:
install: remediation: - retries: -1 + retries: 3 + remediateLastFailure: true upgrade: force: true remediation: - retries: -1 + strategy: rollback + retries: 3 + remediateLastFailure: truepkg/registry/apps/application/rest.go (2)
1031-1031: Avoid hardcoding the reconcile interval (make it configurable).Set via config (e.g., ReleaseConfig) or a package-level default constant so operators can tune it without code changes.
1033-1041: -1 is valid (means unlimited retries); prefer a finite default or make it configurableHelmRelease v2 accepts negative integers for spec.install.remediation.retries and spec.upgrade.remediation.retries; -1 = unlimited retries. To avoid indefinite churn on persistent failures, use a finite retry count (3–5) or expose this as a configuration option.
Optionally apply a finite default:
- Remediation: &helmv2.InstallRemediation{ - Retries: -1, - }, + Remediation: &helmv2.InstallRemediation{ + Retries: 3, + },- Remediation: &helmv2.UpgradeRemediation{ - Retries: -1, - }, + Remediation: &helmv2.UpgradeRemediation{ + Retries: 3, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
packages/apps/Makefile(1 hunks)packages/apps/bucket/templates/helmrelease.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/cert-manager-crds.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/cert-manager.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/cilium.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/coredns.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/fluxcd.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/gateway-api-crds.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/gpu-operator.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/ingress-nginx.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/monitoring-agents.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/velero.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/victoria-metrics-operator.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/volumesnapshot_crd.yaml(1 hunks)packages/apps/nats/templates/nats.yaml(1 hunks)packages/apps/tenant/templates/etcd.yaml(1 hunks)packages/apps/tenant/templates/info.yaml(1 hunks)packages/apps/tenant/templates/ingress.yaml(1 hunks)packages/apps/tenant/templates/monitoring.yaml(1 hunks)packages/apps/tenant/templates/seaweedfs.yaml(1 hunks)packages/core/platform/templates/apps.yaml(1 hunks)packages/extra/ingress/templates/nginx-ingress.yaml(1 hunks)packages/extra/seaweedfs/templates/seaweedfs.yaml(1 hunks)pkg/registry/apps/application/rest.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/apps/kubernetes/templates/helmreleases/cert-manager-crds.yaml
- packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml
- packages/apps/kubernetes/templates/helmreleases/victoria-metrics-operator.yaml
- packages/apps/kubernetes/templates/helmreleases/volumesnapshot_crd.yaml
- packages/apps/kubernetes/templates/helmreleases/gateway-api-crds.yaml
- packages/apps/bucket/templates/helmrelease.yaml
- packages/apps/tenant/templates/info.yaml
- packages/core/platform/templates/apps.yaml
- packages/apps/kubernetes/templates/helmreleases/velero.yaml
- packages/apps/kubernetes/templates/helmreleases/ingress-nginx.yaml
- packages/extra/ingress/templates/nginx-ingress.yaml
- packages/apps/kubernetes/templates/helmreleases/gpu-operator.yaml
- packages/apps/tenant/templates/seaweedfs.yaml
- packages/apps/tenant/templates/etcd.yaml
- packages/apps/kubernetes/templates/helmreleases/fluxcd.yaml
- packages/apps/kubernetes/templates/helmreleases/cilium.yaml
- packages/apps/kubernetes/templates/helmreleases/monitoring-agents.yaml
- packages/extra/seaweedfs/templates/seaweedfs.yaml
🧰 Additional context used
🪛 checkmake (0.2.2)
packages/apps/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (8)
packages/apps/kubernetes/templates/helmreleases/coredns.yaml (1)
31-32: LGTM: interval/timeout placementDefining interval and timeout under spec (HelmRelease v2) is correct.
packages/apps/kubernetes/templates/helmreleases/cert-manager.yaml (1)
26-27: LGTM: interval/timeout under specPlacement and values look fine for HelmRelease v2.
packages/apps/nats/templates/nats.yaml (1)
39-40: Durations LGTM
interval: 5mandtimeout: 10mare valid Flux durations and consistent with similar changes elsewhere.packages/apps/tenant/templates/monitoring.yaml (1)
21-22: Top‑level interval/timeout look correct; verify cadence vs. source updates.These control HelmRelease reconciliation and action timeout. If you also want a distinct chart artifact refresh cadence, set
spec.chart.spec.interval. Ensure this 5m interval aligns with your HelmRepository index update to avoid unnecessary reconciles.packages/apps/Makefile (2)
1-1: OUT path fix looks correct.Matches repo-root _out layout used elsewhere.
6-9: Create OUT dir before helm package; current recipe will fail if OUT doesn’t exist.helm doesn’t create the destination directory. Also safer to re-create OUT after rm.
Apply:
repo: - rm -rf "$(OUT)" - helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION) - helm repo index "$(OUT)" + rm -rf -- "$(OUT)" + mkdir -p -- "$(OUT)" + helm package -d "$(OUT)" $(CHARTS) --version "$(COZYSTACK_VERSION)" + helm repo index "$(OUT)"packages/apps/tenant/templates/ingress.yaml (1)
21-22: No change required — HelmReleases use interval: 5m / timeout: 10m consistentlyRepo search shows multiple HelmRelease templates set interval: 5m and timeout: 10m; differing intervals in ServiceMonitor/PodMonitor manifests (30s/1m, etc.) are expected.
pkg/registry/apps/application/rest.go (1)
1022-1022: LGTM: broad chart version range (includes pre-releases).Confirmed: Helm semver ">= 0.0.0-0" includes pre-release versions and is commonly used to match all versions; keep as-is to include prereleases, or use ">= 0.0.0" to exclude them.
|
|
||
| fix-chartnames: | ||
| find . -maxdepth 2 -name Chart.yaml | awk -F/ '{print $$2}' | while read i; do sed -i "s/^name: .*/name: $$i/" "$$i/Chart.yaml"; done | ||
| include ../../scripts/common-envs.mk |
There was a problem hiding this comment.
🧩 Analysis chain
Verify include path and COZYSTACK_VERSION definition.
Ensure the included file exists and defines COZYSTACK_VERSION, or repo target will fail.
🏁 Script executed:
#!/bin/bash
set -e
test -f ../../scripts/common-envs.mk || { echo "missing: ../../scripts/common-envs.mk"; exit 1; }
rg -n -C0 --fixed-strings 'COZYSTACK_VERSION' ../../scripts/common-envs.mk || { echo "COZYSTACK_VERSION not defined"; exit 1; }Length of output: 37
Fix missing include and ensure COZYSTACK_VERSION is defined
packages/apps/Makefile (line 4) references ../../scripts/common-envs.mk but that file is missing; add the file at that path or update the include, and ensure it defines COZYSTACK_VERSION (or set COZYSTACK_VERSION locally) to avoid build/target failures.
🤖 Prompt for AI Agents
In packages/apps/Makefile around line 4 the include reference to
../../scripts/common-envs.mk is invalid/missing which causes COZYSTACK_VERSION
to be undefined; either add the referenced file at ../../scripts/common-envs.mk
and ensure it exports COZYSTACK_VERSION, or update the include path to the
correct location of common-envs.mk, or alternatively set COZYSTACK_VERSION
directly in packages/apps/Makefile (e.g. COZYSTACK_VERSION := <version>) so the
variable is always defined for builds/targets.
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
f0cf7b3 to
9f9d8f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/apps/kubernetes/templates/helmreleases/victoria-metrics-operator.yaml (1)
33-33: Be cautious with upgrade.force: true (can recreate resources and cause downtime).Consider gating this via a value or enabling only where needed to avoid disruptive rollouts.
Apply this change to make it configurable:
- force: true + force: {{ .Values.addons.monitoringAgents.upgradeForce | default false }}packages/apps/kubernetes/templates/helmreleases/velero.yaml (1)
33-33: Avoid unconditional upgrade.force for backup/restore controllers.Velero upgrades with --force may delete/recreate hooks and CRDs, potentially interrupting backups. Prefer making it opt-in.
- force: true + force: {{ .Values.addons.velero.upgradeForce | default false }}Please confirm your Helm Controller version supports templated booleans here (it should for v2 CRD).
packages/apps/kubernetes/templates/helmreleases/cert-manager.yaml (1)
33-33: For cert-manager, --force can recreate webhooks and API resources; risk of issuance disruption.Recommend gating or leaving false by default, enabling only during specific migrations.
- force: true + force: {{ .Values.addons.certManager.upgradeForce | default false }}Ensure rollout plans account for brief webhook downtime if force is enabled.
packages/apps/kubernetes/templates/helmreleases/ingress-nginx.yaml (1)
52-52: Ingress with upgrade.force: true may cause transient routing interruptions.Suggest making it configurable and defaulting to false unless resolving immutable field errors.
- force: true + force: {{ .Values.addons.ingressNginx.upgradeForce | default false }}If you intend to force-recreate, consider scheduling windows or surge capacity to reduce impact.
packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml (1)
61-61: upgrade.force: true can briefly disrupt recommender/updater components.Make it opt-in via values to control rollouts.
- force: true + force: {{ .Values.addons.verticalPodAutoscaler.upgradeForce | default false }}Confirm no dependent workloads rely on uninterrupted VPA recommendations during upgrades.
packages/apps/Makefile (3)
2-2: Harden chart discovery (avoid root matches; stable order).Use mindepth and sort for determinism and to avoid accidental “./Chart.yaml” picks.
Apply this diff:
-CHARTS := $(shell find . -maxdepth 2 -name Chart.yaml | awk -F/ '{print $$2}') +CHARTS := $(shell find . -mindepth 2 -maxdepth 2 -name Chart.yaml | awk -F/ '{print $$2}' | sort)
11-12: Reuse CHARTS and avoid re-running find; note sed -i portability.
- Use $(CHARTS) for consistency and speed.
- BSD sed (macOS) needs
-i ''; if local dev targets macOS, consider guarding or using GNU sed in tooling containers.Apply this diff:
fix-charts: - find . -maxdepth 2 -name Chart.yaml | awk -F/ '{print $$2}' | while read i; do sed -i -e "s/^name: .*/name: $$i/" -e "s/^version: .*/version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process/g" "$$i/Chart.yaml"; done + for i in $(CHARTS); do \ + sed -i -e "s/^name: .*/name: $$i/" \ + -e "s/^version: .*/version: 0.0.0 # Placeholder, the actual version will be automatically set during the build process/g" \ + "$$i/Chart.yaml"; \ + doneOptional: prefer yq for YAML-safe edits if available:
- yq e -i '.name = strenv(i) | .version = "0.0.0"' "$i/Chart.yaml"
1-2: Add basic phony/default/clean targets (checkmake warnings).Not required for functionality, but improves DX and CI linting.
Add:
.PHONY: all repo fix-charts clean all: repo clean: rm -rf "$(OUT)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.pre-commit-config.yaml(0 hunks)packages/apps/Makefile(1 hunks)packages/apps/bucket/templates/helmrelease.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/cert-manager-crds.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/cert-manager.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/cilium.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/coredns.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/fluxcd.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/gateway-api-crds.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/gpu-operator.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/ingress-nginx.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/monitoring-agents.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/velero.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/victoria-metrics-operator.yaml(1 hunks)packages/apps/kubernetes/templates/helmreleases/volumesnapshot_crd.yaml(1 hunks)packages/apps/nats/templates/nats.yaml(1 hunks)packages/apps/tenant/templates/etcd.yaml(1 hunks)packages/apps/tenant/templates/info.yaml(1 hunks)packages/apps/tenant/templates/ingress.yaml(1 hunks)packages/apps/tenant/templates/monitoring.yaml(1 hunks)packages/apps/tenant/templates/seaweedfs.yaml(1 hunks)packages/core/platform/templates/apps.yaml(1 hunks)packages/extra/ingress/templates/nginx-ingress.yaml(1 hunks)packages/extra/seaweedfs/templates/seaweedfs.yaml(1 hunks)pkg/registry/apps/application/rest.go(2 hunks)
💤 Files with no reviewable changes (1)
- .pre-commit-config.yaml
🚧 Files skipped from review as they are similar to previous changes (19)
- packages/apps/tenant/templates/ingress.yaml
- packages/apps/bucket/templates/helmrelease.yaml
- pkg/registry/apps/application/rest.go
- packages/apps/kubernetes/templates/helmreleases/monitoring-agents.yaml
- packages/core/platform/templates/apps.yaml
- packages/apps/tenant/templates/info.yaml
- packages/apps/kubernetes/templates/helmreleases/gateway-api-crds.yaml
- packages/apps/kubernetes/templates/helmreleases/gpu-operator.yaml
- packages/apps/kubernetes/templates/helmreleases/cilium.yaml
- packages/apps/nats/templates/nats.yaml
- packages/apps/kubernetes/templates/helmreleases/coredns.yaml
- packages/extra/seaweedfs/templates/seaweedfs.yaml
- packages/extra/ingress/templates/nginx-ingress.yaml
- packages/apps/kubernetes/templates/helmreleases/cert-manager-crds.yaml
- packages/apps/kubernetes/templates/helmreleases/volumesnapshot_crd.yaml
- packages/apps/tenant/templates/seaweedfs.yaml
- packages/apps/tenant/templates/etcd.yaml
- packages/apps/tenant/templates/monitoring.yaml
- packages/apps/kubernetes/templates/helmreleases/fluxcd.yaml
🧰 Additional context used
🪛 checkmake (0.2.2)
packages/apps/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (8)
packages/apps/kubernetes/templates/helmreleases/victoria-metrics-operator.yaml (1)
26-27: LGTM: interval/timeout placement under spec is correct for HelmRelease v2.packages/apps/kubernetes/templates/helmreleases/velero.yaml (1)
26-27: LGTM: interval/timeout under spec are valid and helpful.packages/apps/kubernetes/templates/helmreleases/cert-manager.yaml (1)
26-27: LGTM: interval/timeout migrated correctly.packages/apps/kubernetes/templates/helmreleases/ingress-nginx.yaml (1)
45-46: LGTM: interval/timeout are set at the correct level.packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml (1)
54-55: LGTM: interval/timeout look good.packages/apps/Makefile (3)
1-1: OUT path fix LGTM.Matches repo-root _out convention and aligns with other packages.
4-4: Verify include exists and COZYSTACK_VERSION is defined.If the file is missing or the var undefined, packaging will fail.
Run:
#!/bin/bash set -euo pipefail test -f ../../scripts/common-envs.mk || { echo "missing: ../../scripts/common-envs.mk"; exit 1; } rg -n --fixed-strings 'COZYSTACK_VERSION' ../../scripts/common-envs.mk || { echo "COZYSTACK_VERSION not defined"; exit 1; }
6-9: Create OUT dir before helm package.After rm -rf, the directory doesn’t exist; helm won’t create it.
Apply this diff:
repo: rm -rf "$(OUT)" + mkdir -p "$(OUT)" helm package -d "$(OUT)" $(CHARTS) --version $(COZYSTACK_VERSION) helm repo index "$(OUT)"
TBD: How can we ensure that migrations were completed before updating user-charts
What this PR does
This PR removes logic for user apps versioning.
It is not needed anymore for new dashboard and does not make sence for cozystack-api server, which always validates values accourding to the latest spec from CozystackResourceDefinition.
Release note
Summary by CodeRabbit
Chores
Refactor