packages/apps/kubernetes/values.yaml (1)

6-10: Consider a less generic key than version

The top-level field name version can easily be confused with the chart’s own .Chart.Version inside templates and with Helm’s built-in Version concept. A more explicit key such as kubernetesVersion would remove that ambiguity without affecting UX (end-users still type one extra word).

packages/apps/kubernetes/values.schema.json (1)

130-142: Schema update correct, but keep it DRY with Makefile/_versions.tpl

The enum/default match the values file – good. However, as pointed out in the Makefile review, maintaining the same list in three locations will quickly drift. Please refactor to source the list from a single definition.

packages/apps/kubernetes/README.md (1)

84-90: Clarify allowed values and patch-level resolution for version.

The description only states “vMAJOR.MINOR”. Because the chart silently expands that value to a full patch version through templates/_versions.tpl, users might be confused why v1.32 becomes v1.32.6, or why v1.27 is rejected.
Consider adding a short note such as:

Only v1.28 – v1.33 are currently supported and will automatically be resolved to the latest patch (see _versions.tpl).

This keeps the docs self-contained and avoids a support round-trip.

packages/apps/kubernetes/templates/_versions.tpl (2)

1-7: Externalise the hard-coded map to ease future upgrades.

Keeping the map inside the template means every patch bump requires editing this file, regenerating the schema enum, and bumping chart/app versions.
A tiny refactor would pull the map into values.yaml (or a dedicated values.versions.yaml) and iterate over it here. The Makefile could then read the same source of truth when regenerating the JSON schema.

Advantages:
• single place to update
• no Helm diff noise in rendered manifests
• easier to back-port security patch versions

Not blocking, but worth considering as version cadence accelerates.

---

3-5: Minor UX: sort supported versions in the failure message.

keys $versionMap returns a random order. A deterministic, sorted list is nicer for the user.

-{{- printf `Kubernetes version %s is not supported, allowed versions are %s` . (keys $versionMap) | fail }}
+{{- $allowed := keys $versionMap | sortAlpha }}
+{{- printf `Kubernetes version %s is not supported, allowed versions are %s` . $allowed | fail }}

packages/apps/kubernetes/templates/cluster.yaml (2)

150-155: Quote the rendered version to appease YAML/Helm linters.

Unquoted plain scalars like v1.32.6 are technically legal YAML, yet several linters flag them as ambiguous. Quoting avoids future CI noise.

-  version: {{ include "kubernetes.versionMap" $.Values.version }}
+  version: "{{ include "kubernetes.versionMap" $.Values.version }}"

---

289-294: Repeat the quoting fix for worker templates.

Same rationale as above – keep manifest style consistent.

-      version: {{ include "kubernetes.versionMap" $.Values.version }}
+      version: "{{ include "kubernetes.versionMap" $.Values.version }}"

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: NickVolynkin
PR: cozystack/cozystack#1117
File: packages/apps/mysql/Makefile:8-8
Timestamp: 2025-06-26T04:29:24.830Z
Learning: The cozystack project uses yq v4+ on their CI runner, so yq v4 syntax (-o json --indent 4) is compatible and version checks are not needed.

hack/e2e-apps/kubernetes.bats (1)

66-66: Enhanced test validation - good improvement.

The updated wait condition now properly validates that the specified version "v1.30" appears in the kamajicontrolplane status, ensuring the version selection feature is working correctly. This is a more robust test than simply checking for resource existence.

Consider adding a comment explaining what the grep is looking for, to improve test maintainability:

+  # Wait for the control plane to show the specified version
   timeout 10 sh -ec 'until kubectl get kamajicontrolplane -n tenant-test kubernetes-test | grep -q "v1.30"; do sleep 1; done'

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/apps/kubernetes/templates/_versions.tpl (1)

3-3: Consider caching the version map for better performance.

The files/versions.yaml file is loaded and parsed every time this template is called. If this template is invoked multiple times within the same chart render, this could lead to redundant file I/O operations.

Consider caching the parsed version map in a variable at the chart level or documenting that this template should only be called once per render.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

hack/e2e-apps/kubernetes-latest-version.bats (3)

4-4: Version extraction assumes ordered keys in YAML file.

The yq 'keys | .[-1]' command assumes that the keys in versions.yaml are ordered and that the last key represents the latest version. This approach is fragile if the YAML file structure changes or if keys are not semantically ordered.

Consider using a more explicit version selection approach:

-LATEST_K8S_VERSION=$(yq 'keys | .[-1]' ../packages/apps/kubernetes/files/versions.yaml)
+LATEST_K8S_VERSION=$(yq 'keys | sort_by(.) | .[-1]' ../packages/apps/kubernetes/files/versions.yaml)

Or better yet, if the versions.yaml file has a specific field for the latest version, use that instead of relying on key ordering.

---

93-93: Port forwarding runs in background without proper cleanup.

The port forwarding command runs in the background with a 40-second timeout, but there's no explicit cleanup of the background process. This could lead to port conflicts in subsequent test runs.

Consider adding explicit cleanup for the port forwarding process:

# Set up port forwarding to the Kubernetes API server for a 40 second timeout
-bash -c 'timeout 40s kubectl port-forward service/kubernetes-test-latest -n tenant-test '"${TEMPORAL_TENANT_PORT}"':6443 > /dev/null 2>&1 &'
+PORT_FORWARD_PID=$(bash -c 'timeout 40s kubectl port-forward service/kubernetes-test-latest -n tenant-test '"${TEMPORAL_TENANT_PORT}"':6443 > /dev/null 2>&1 & echo $!')

And add cleanup before the final resource deletion:

+# Clean up port forwarding
+kill $PORT_FORWARD_PID 2>/dev/null || true
+
# Clean up by deleting the Kubernetes resource

---

96-96: Version verification could be more robust.

The version verification uses grep to match the server version, but this approach might be fragile if the output format changes. Consider using more specific JSON output parsing.

-timeout 20 sh -ec 'until kubectl --kubeconfig tenantkubeconfig version 2>/dev/null | grep "Server Version: $LATEST_K8S_VERSION"; do sleep 5; done'
+timeout 20 sh -ec 'until kubectl --kubeconfig tenantkubeconfig version -o json 2>/dev/null | jq -r ".serverVersion.gitVersion" | grep -q "^$LATEST_K8S_VERSION"; do sleep 5; done'

hack/e2e-apps/kubernetes-previous-version.bats (2)

4-5: Misleading comment about version extraction.

The comment states "Get latest version of k8s from gcr" but the code actually extracts the previous version (second-to-last key) from the local versions.yaml file.

-# Get latest version of k8s from gcr
-PREVIOUS_K8S_VERSION=$(yq 'keys | .[-2]' ../packages/apps/kubernetes/files/versions.yaml)
+# Get previous version of k8s from versions.yaml
+PREVIOUS_K8S_VERSION=$(yq 'keys | .[-2]' ../packages/apps/kubernetes/files/versions.yaml)

---

5-5: Version extraction assumes at least two versions exist.

The yq 'keys | .[-2]' command assumes there are at least two versions in the versions.yaml file. This could fail if only one version is defined.

Consider adding validation to ensure at least two versions exist:

-PREVIOUS_K8S_VERSION=$(yq 'keys | .[-2]' ../packages/apps/kubernetes/files/versions.yaml)
+VERSION_COUNT=$(yq 'keys | length' ../packages/apps/kubernetes/files/versions.yaml)
+if [ "$VERSION_COUNT" -lt 2 ]; then
+  echo "Error: At least two versions required in versions.yaml for previous version test"
+  exit 1
+fi
+PREVIOUS_K8S_VERSION=$(yq 'keys | sort_by(.) | .[-2]' ../packages/apps/kubernetes/files/versions.yaml)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

hack/e2e-apps/kubernetes.bats (1)

98-98: Potential shell escaping issue with version string in grep.

The version string ${k8s_version} could contain characters that have special meaning in grep patterns. Consider using grep -F (fixed string) or fgrep for literal matching.

Apply this improvement:

-  timeout 20 sh -ec 'until kubectl --kubeconfig tenantkubeconfig version 2>/dev/null | grep -q "Server Version: ${k8s_version}"; do sleep 5; done'
+  timeout 20 sh -ec 'until kubectl --kubeconfig tenantkubeconfig version 2>/dev/null | grep -Fq "Server Version: ${k8s_version}"; do sleep 5; done'

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/apps/postgres/values.yaml (2)

90-95: Remove trailing whitespace & align example indentation

Line 95 has trailing spaces flagged by yamllint and should be trimmed.
While touching the block, consider indenting the example keys two more spaces so they are siblings of cpu/memory under resources: (current comment implies that but indentation shows them as top-level).

 # resources:
-#   cpu: 4000m
-#   memory: 4Gi␠
+#   cpu: 4000m
+#   memory: 4Gi

---

96-97: Ensure allowed preset values are schema-validated

The comment now enumerates the permitted presets. Double-check that values.schema.json (or _helpers.tpl validation) constrains resourcesPreset to the same list to prevent typos sneaking through Helm.
If such validation does not yet exist, consider adding it.

packages/apps/mysql/values.yaml (1)

58-65: Remove trailing whitespace to satisfy YAMLlint
The blank line after the commented example (current line 63) contains stray spaces flagged by YAMLlint (trailing-spaces).

- #   memory: 4Gi␠␠
+ #   memory: 4Gi
+

.github/workflows/tags.yaml (3)

115-121: Prefer using the built-in GITHUB_TOKEN or disable credential persistence when injecting a PAT

Embedding ${GH_PAT} directly in the remote URL works, but it introduces a second long-lived secret into the workflow.
Unless you need scopes that the short-lived GITHUB_TOKEN cannot provide (e.g. pushing to protected branches), you can drop the PAT entirely and rely on GITHUB_TOKEN with the already-declared contents: write permission.

If keeping the PAT is truly required, add persist-credentials: false to the earlier actions/checkout step so the default token isn’t written to .git/config, avoiding accidental credential mix-ups:

-      - name: Checkout code
+      - name: Checkout code
         if: steps.check_release.outputs.skip == 'false'
         uses: actions/checkout@v4
         with:
           fetch-depth: 0
           fetch-tags:  true
+          persist-credentials: false   # avoid storing the default token

---

195-201: Duplicate git-config boilerplate – consider a reusable composite action

The user name / email configuration and the remote-URL overwrite are repeated here and in the “Commit release artifacts” step.
Encapsulating this into a small composite action (or at least a reusable YAML step via uses: ./.github/actions/git-bot-setup) removes duplication and keeps future updates in one place.

---

210-211: PAT scopes check

Switching the github-token input to ${{ secrets.GH_PAT }} gives the script full PAT permissions.
Double-check that the token is repository-scoped and limited to the minimal scopes required for PR creation (repo, workflow if needed) to reduce blast radius if it leaks.

packages/apps/vpn/README.md (3)

3-4: Add missing article for grammatical correctness

The phrase “deployment and management of VPN server” is missing the article “a”.

-Managed VPN Service simplifies the deployment and management of VPN server, enabling you to establish secure connections with ease.
+Managed VPN Service simplifies the deployment and management of a VPN server, enabling you to establish secure connections with ease.

---

6-6: Replace bare URL with a Markdown link

Stops the markdown-lint MD034 warning and keeps formatting consistent.

-- VPN client applications: https://shadowsocks5.github.io/en/download/clients.html
+- [VPN client applications](https://shadowsocks5.github.io/en/download/clients.html)

---

10-13: Consolidate sentences and add canonical link to Outline Server

The four short sentences read a bit staccato. Merging them, adding a link to the upstream project, and removing the redundant “Internally known as” clause improves flow and clarity.

-The VPN Service is powered by the Outline Server, an advanced and user-friendly VPN solution.
-Internally known as "Shadowbox", which simplifies the process of setting up and sharing Shadowsocks servers.
-It operates by launching Shadowsocks instances on demand.
-Furthermore, Shadowbox is compatible with standard Shadowsocks clients, providing flexibility and ease of use for your VPN requirements.
+The VPN service is powered by [Outline Server](https://github.com/Jigsaw-Code/outline-server) (internally called **Shadowbox**).  
+Shadowbox spins up Shadowsocks instances on demand and is fully compatible with any standard Shadowsocks client, giving you maximum flexibility.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: NickVolynkin
PR: cozystack/cozystack#1120
File: packages/apps/clickhouse/README.md:60-67
Timestamp: 2025-07-03T05:54:51.264Z
Learning: The `cozy-lib.resources.sanitize` function in packages/library/cozy-lib/templates/_resources.tpl supports both standard Kubernetes resource format (with limits:/requests: sections) and flat format (direct resource specifications). The flat format takes priority over nested values. CozyStack apps include cozy-lib as a chart dependency through symlinks in packages/apps/*/charts/cozy-lib directories.

hack/e2e-apps/kubernetes.bats (1)

70-106: Comprehensive test implementation addressing past feedback.

The implementation addresses the previous review comment by verifying that the Kubernetes API reports the correct version (line 98). The test includes proper wait conditions, timeouts, and cleanup.

However, consider these improvements:

1. Error handling: The test could benefit from better error handling for the port forwarding and version verification steps.
2. Resource cleanup: Consider adding cleanup in a trap to ensure resources are cleaned up even if the test fails.

+ # Add cleanup trap
+ cleanup() {
+   kubectl -n tenant-test delete kuberneteses.apps.cozystack.io $test_name --ignore-not-found=true
+ }
+ trap cleanup EXIT

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

packages/apps/kubernetes/templates/_versions.tpl (2)

2-2: Consider removing unnecessary empty lines for cleaner formatting.

The empty lines at lines 2 and 7 don't add significant value and could be removed for more compact formatting.

 {{- define "kubernetes.versionMap" }}
-
 {{- $versionMap := .Files.Get "files/versions.yaml" | fromYaml }}
 {{- if not (hasKey $versionMap .Values.version) }}
     {{- printf `Kubernetes version %s is not supported, allowed versions are %s` $.Values.version (keys $versionMap) | fail }}
 {{- end }}
-
 {{- index $versionMap .Values.version }}
 {{- end }}

Also applies to: 7-7

---

5-5: Verify the error message format for readability.

The error message concatenates the version list directly, which may not be very readable for users. Consider formatting the allowed versions as a comma-separated list.

-    {{- printf `Kubernetes version %s is not supported, allowed versions are %s` $.Values.version (keys $versionMap) | fail }}
+    {{- printf `Kubernetes version %s is not supported, allowed versions are: %s` $.Values.version (keys $versionMap | join ", ") | fail }}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: lllamnyp
PR: cozystack/cozystack#1160
File: packages/system/hetzner-robotlb/charts/robotlb/templates/role.yaml:6-8
Timestamp: 2025-07-11T06:28:13.696Z
Learning: In Helm templates, the `{{-` directive chomps all leading whitespace including newlines back to the previous content, so `{{- toYaml .Values.something | nindent 2 }}` will render correctly even with apparent indentation issues. However, for better style, it's cleaner to put the template directive on the same line as the parent key (e.g., `rules: {{- toYaml .Values.serviceAccount.permissions | nindent 2 }}`).