Remove user-facing config of limits and requests #935
Conversation
## Walkthrough
This change introduces a new Helm library chart `cozy-lib` with resource preset and sanitization templates. The ClickHouse application chart is updated to depend on this library and use its resource helpers. Makefiles for `apps`, `extra`, and the new `library` package are refactored to delegate packaging to a new script and include version map checks and repository generation for the library chart.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------------------------------------------|----------------|
| Makefile, packages/apps/Makefile, packages/extra/Makefile | Updated `repos` targets to include `packages/library` for version map checks and repo packaging. Refactored `repo` targets to call the new packaging script with dynamic temp directories. |
| packages/apps/clickhouse/Chart.yaml, packages/apps/clickhouse/charts/cozy-lib | Bumped ClickHouse chart version to 0.9.0, added `cozy-lib` as a dependency, and referenced it in the charts directory. |
| packages/apps/clickhouse/templates/clickhouse.yaml | Changed resource configuration to use `cozy-lib` Helm helpers for resource sanitization and presets. |
| packages/apps/versions_map | Updated ClickHouse version from 0.8.0 to 0.9.0. |
| packages/library/Makefile, packages/library/versions_map | Added new Makefile and versions_map for the `cozy-lib` library chart, supporting packaging, version map generation, and repo creation. |
| packages/library/cozy-lib/.helmignore, packages/library/cozy-lib/Chart.yaml, packages/library/cozy-lib/Makefile, packages/library/cozy-lib/README.md, packages/library/cozy-lib/templates/_resourcepresets.tpl, packages/library/cozy-lib/templates/_resources.tpl, packages/library/cozy-lib/values.schema.json, packages/library/cozy-lib/values.yaml | Added new Helm library chart `cozy-lib` with resource preset and sanitization templates, chart metadata, ignore rules, schema, documentation, and a Makefile target for README generation. |
| hack/package_chart.sh | Added new shell script to automate Helm chart packaging from git archives, supporting optional dependency extraction. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant MainMakefile
participant LibraryMakefile
participant Git
participant Helm
participant AppsMakefile
participant ExtraMakefile
participant ClickHouseChart
User->>MainMakefile: make repos
MainMakefile->>LibraryMakefile: make -C packages/library check-version-map
LibraryMakefile->>Git: git diff --exit-code versions_map
MainMakefile->>LibraryMakefile: make -C packages/library repo
LibraryMakefile->>Git: git archive cozy-lib@commit
LibraryMakefile->>Helm: helm package cozy-lib
MainMakefile->>AppsMakefile: make -C packages/apps repo
AppsMakefile->>Git: git archive clickhouse@commit (+ cozy-lib if exists)
AppsMakefile->>Helm: helm package clickhouse (with cozy-lib dependency)
MainMakefile->>ExtraMakefile: make -C packages/extra repo
ExtraMakefile->>Git: git archive extra@commit (+ cozy-lib if exists)
ExtraMakefile->>Helm: helm package extraPossibly related PRs
Suggested labels
Suggested reviewers
Poem
|
df07e7b to
c19f470
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
packages/apps/Makefile (1)
25-26:⚠️ Potential issueUpdate version-map file reference for apps.
Thecheck-version-maptarget still diffsversions_map, but your loop reads fromapps/versions_map2. To keep consistency and avoid false failures, change it to:@@ check-version-map: gen-versions-map - git diff --exit-code -- versions_map + git diff --exit-code -- versions_map2🧰 Tools
🪛 GitHub Actions: Pull Request
[error] 26-26: Makefile target 'image-cozystack' failed due to failure in 'repos' target.
[error] 26-26: Top-level make build failed with exit code 2 due to errors in sub-make targets.
🧹 Nitpick comments (6)
packages/library/cozy-lib/Makefile (1)
4-6: Declare generate as a phony target
To avoid conflicts with files namedgenerate, it’s best practice to mark it.PHONY. For example:+ .PHONY: generate generate: readme-generator -v values.yaml -s values.schema.json -r README.mdpackages/library/Makefile (3)
1-2: Add phony targets and enforce strict shell flags.
It’s best practice to declare your Makefile targets as.PHONYand enable-euo pipefailso that any command failure aborts the recipe immediately. For example:+ .PHONY: repo fix-chartnames gen-versions-map check-version-map + .SHELLFLAGS += -eu -o pipefail
4-11: Clean up the temporary directory after packaging.
You create$(TMP)viamktemp -dbut never remove it. This can leave orphaned directories on repeated runs. Consider adding a cleanup step at the end of therepotarget:@@ repo: - done + done + rm -rf "$(TMP)"
13-14: Future-prooffix-chartnamesdepth.
Thefind . -maxdepth 2will catch current charts underpackages/library/<chart>, but if in future charts live deeper (e.g., nested libraries), they’ll be skipped. You may want to remove-maxdepth 2or adjust it, or explicitly document this constraint.packages/apps/Makefile (2)
2-2: Ensure temp directory is torn down.
You allocateTMP := $(shell mktemp -d)but never delete it. Add a remove step (rm -rf "$(TMP)") after your loop in therepotarget to avoid leftover directories.
1-3: Declare phony targets and strict shell settings.
Mirroring the library Makefile, add at the top:+ .PHONY: repo fix-chartnames gen-versions-map check-version-map + .SHELLFLAGS += -eu -o pipefailThis will catch errors early and avoid accidental file collisions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Makefile(1 hunks)packages/apps/Makefile(1 hunks)packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/charts/cozy-lib(1 hunks)packages/apps/clickhouse/templates/clickhouse.yaml(1 hunks)packages/apps/versions_map(1 hunks)packages/library/Makefile(1 hunks)packages/library/cozy-lib/.helmignore(1 hunks)packages/library/cozy-lib/Chart.yaml(1 hunks)packages/library/cozy-lib/Makefile(1 hunks)packages/library/cozy-lib/README.md(1 hunks)packages/library/cozy-lib/templates/_resourcepresets.tpl(1 hunks)packages/library/cozy-lib/templates/_resources.tpl(1 hunks)packages/library/cozy-lib/values.schema.json(1 hunks)packages/library/cozy-lib/values.yaml(1 hunks)packages/library/versions_map(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pull Request
packages/library/cozy-lib/values.yaml
[error] 1-1: Makefile target 'check-version-map' failed due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
packages/library/cozy-lib/README.md
[error] 1-1: Makefile target 'check-version-map' failed due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
packages/library/versions_map
[error] 1-2: Makefile check-version-map failed due to unexpected diff in versions_map file.
packages/library/cozy-lib/values.schema.json
[error] 1-1: Makefile target 'check-version-map' failed 8000 due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
packages/library/cozy-lib/templates/_resourcepresets.tpl
[error] 1-1: Makefile target 'check-version-map' failed due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
packages/library/cozy-lib/Chart.yaml
[error] 1-1: Makefile target 'check-version-map' failed due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
packages/library/cozy-lib/Makefile
[error] 20-20: Makefile target 'check-version-map' failed due to missing Chart.yaml file and versions_map diff mismatch.
[error] 31-31: Makefile target 'repos' failed due to failure in 'check-version-map' target.
[error] 26-26: Makefile target 'image-cozystack' failed due to failure in 'repos' target.
[error] 26-26: Top-level make build failed with exit code 2 due to errors in sub-make targets.
Makefile
[error] 20-20: Makefile target 'check-version-map' failed due to missing Chart.yaml file and versions_map diff mismatch.
[error] 31-31: Makefile target 'repos' failed due to failure in 'check-version-map' target.
[error] 26-26: Makefile target 'image-cozystack' failed due to failure in 'repos' target.
[error] 26-26: Top-level make build failed with exit code 2 due to errors in sub-make targets.
packages/library/cozy-lib/templates/_resources.tpl
[error] 1-1: Makefile target 'check-version-map' failed due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
packages/library/cozy-lib/.helmignore
[error] 1-1: Makefile target 'check-version-map' failed due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
packages/library/Makefile
[error] 20-20: Makefile target 'check-version-map' failed due to missing Chart.yaml file and versions_map diff mismatch.
[error] 31-31: Makefile target 'repos' failed due to failure in 'check-version-map' target.
[error] 26-26: Makefile target 'image-cozystack' failed due to failure in 'repos' target.
[error] 26-26: Top-level make build failed with exit code 2 due to errors in sub-make targets.
packages/apps/Makefile
[error] 20-20: Makefile target 'check-version-map' failed due to missing Chart.yaml file and versions_map diff mismatch.
[error] 31-31: Makefile target 'repos' failed due to failure in 'check-version-map' target.
[error] 26-26: Makefile target 'image-cozystack' failed due to failure in 'repos' target.
[error] 26-26: Top-level make build failed with exit code 2 due to errors in sub-make targets.
🔇 Additional comments (18)
packages/library/cozy-lib/README.md (1)
1-2: Verify Chart.yaml presence in cozy-lib.
The pipeline failure indicates a missingChart.yaml. Ensure thatpackages/library/cozy-lib/Chart.yamlexists and is committed so thecheck-version-mapand packaging targets pass.🧰 Tools
🪛 GitHub Actions: Pull Request
[error] 1-1: Makefile target 'check-version-map' failed due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
packages/apps/versions_map (1)
12-12: Approve clickhouse version bump to 0.9.0.
The mappingclickhouse 0.9.0 HEADaligns with the updated Helm chart version inpackages/apps/clickhouse/Chart.yaml. No further action required.packages/library/cozy-lib/values.yaml (1)
1-1: Empty values.yaml is intentional.
An empty object invalues.yamlfits the minimal configuration schema for this library chart. Ensure any future default values are declared invalues.schema.jsonand documented in the README.🧰 Tools
🪛 GitHub Actions: Pull Request
[error] 1-1: Makefile target 'check-version-map' failed due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
packages/library/versions_map (1)
1-1:✅ Verification successful
Ensure versions_map matches Chart.yaml version.
The entrycozy-lib 0.1.0 HEADmust match theversion:field inpackages/library/cozy-lib/Chart.yaml. Verify that Chart.yaml declaresversion: 0.1.0so thecheck-version-mapMakefile target succeeds.Run:
🏁 Script executed:
#!/bin/bash # Verify Chart.yaml version matches versions_map grep '^version:' packages/library/cozy-lib/Chart.yamlLength of output: 70
versions_map and Chart.yaml versions are in sync
The entry
cozy-lib 0.1.0 HEADmatches theversion: 0.1.0inpackages/library/cozy-lib/Chart.yaml, so thecheck-version-mapMakefile target will succeed.🧰 Tools
🪛 GitHub Actions: Pull Request
[error] 1-2: Makefile check-version-map failed due to unexpected diff in versions_map file.
packages/library/cozy-lib/values.schema.json (2)
1-5: Initialize empty schema for library chart
Defining an emptypropertiesobject is acceptable for bootstrapping the chart’s schema. You can populate this schema with the top-level keys (e.g.,resources.cpu,resources.memory) once the sanitization and preset templates expose parameters.🧰 Tools
🪛 GitHub Actions: Pull Request
[error] 1-1: Makefile target 'check-version-map' failed due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
1-5:✅ Verification successful
Validate presence of Chart.yaml for the library
The documentation‐generation and version‐mapping targets rely on aChart.yamlin this directory. Please ensurepackages/library/cozy-lib/Chart.yamlexists, or packaging and CI will fail. You can verify with:
🏁 Script executed:
#!/bin/bash # Verify Chart.yaml exists for cozy-lib if [ ! -f "packages/library/cozy-lib/Chart.yaml" ]; then echo "ERROR: packages/library/cozy-lib/Chart.yaml is missing." >&2 exit 1 fiLength of output: 53
Chart.yaml presence verified
Verified thatpackages/library/cozy-lib/Chart.yamlexists and CI/packaging targets will succeed. No further action required.🧰 Tools
🪛 GitHub Actions: Pull Request
[error] 1-1: Makefile target 'check-version-map' failed due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
packages/apps/clickhouse/Chart.yaml (2)
19-20: Ensure version bump consistency with versions_map
You’ve updated the chart version to0.9.0. Confirm that this change is mirrored inpackages/apps/versions_map(and any other version‐mapping files) so thatmake check-version-mappasses.
27-30: Declare new library dependency
Addingcozy-libas a library dependency is correct. Verify that the chart repository URL is accessible and that the library is published at0.1.0.packages/library/cozy-lib/Makefile (1)
1-2: Include shared build logic
Pulling incommon-envs.mkandpackage.mkis appropriate for standardizing package workflows.packages/library/cozy-lib/.helmignore (1)
1-24:.helmignoreis comprehensive
The ignore patterns cover common VCS artifacts, backups, and IDE files, which is exactly what we need to keep the chart packages clean.🧰 Tools
🪛 GitHub Actions: Pull Request
[error] 1-1: Makefile target 'check-version-map' failed due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
packages/library/cozy-lib/templates/_resourcepresets.tpl (2)
1-49: Resource presets implementation looks good!This new template provides a well-structured set of predefined resource configurations ranging from "nano" to "2xlarge". The implementation is consistent with the PR objectives - CPU is defined only in requests but not in limits, which aligns with the goal of setting limits equal to requests except for CPU.
The error handling for invalid presets is robust, and the documentation clearly indicates these presets are intended for basic testing rather than production use.
🧰 Tools
🪛 GitHub Actions: Pull Request
[error] 1-1: Makefile target 'check-version-map' failed due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
1-49:❌ Incorrect review comment
Pipeline failure needs to be addressed
The pipeline is failing with an error related to the 'check-version-map' target due to a missing Chart.yaml file. This needs to be resolved before merging.
🏁 Script executed:
#!/bin/bash # Check if the Chart.yaml file exists in the expected location for the version map cd packages/library echo "Current directory: $(pwd)" find . -name "versions_map" -type f -exec cat {} \; echo "Checking for Chart.yaml files in library subdirectories:" find . -name "Chart.yaml" -type fLength of output: 401
Chart.yaml found — check pipeline configuration
The Chart.yaml file is present at
packages/library/cozy-lib/Chart.yaml. The failure in thecheck-version-maptarget reflects a path/configuration mismatch, not a missing file. Please update the version-map logic or pipeline settings to point to the correct location.• Chart.yaml location:
packages/library/cozy-lib/Chart.yamlLikely an incorrect or invalid review comment.
🧰 Tools
🪛 GitHub Actions: Pull Request
[error] 1-1: Makefile target 'check-version-map' failed due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
packages/library/cozy-lib/Chart.yaml (1)
1-18: Chart definition is correctly configured as a library chartThe Chart.yaml file properly defines a Helm library chart with appropriate metadata. Using a library chart for shared templates is a good architectural decision that promotes code reuse.
🧰 Tools
🪛 GitHub Actions: Pull Request
[error] 1-1: Makefile target 'check-version-map' failed due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
packages/apps/clickhouse/templates/clickhouse.yaml (2)
125-125: Resource sanitization integration looks goodThe use of the new
cozy-lib.resources.sanitizetemplate is properly integrated here for handling resources when explicitly defined.
127-127: Resource presets integration looks goodThe use of the new
cozy-lib.resources.presettemplate for handling resource presets simplifies the template and standardizes resource handling across the codebase.packages/library/cozy-lib/templates/_resources.tpl (2)
1-53: Resource sanitization implementation is well-designedThe
cozy-lib.resources.sanitizetemplate is well-implemented with comprehensive documentation and examples. The logic properly:
- Consolidates inputs from limits, requests, and top-level entries
- Implements the intended handling where CPU has only requests (no limits)
- Ensures all other resources have identical values for requests and limits
This implementation aligns perfectly with the PR objectives of maintaining backward compatibility while simplifying resource configuration.
🧰 Tools
🪛 GitHub Actions: Pull Request
[error] 1-1: Makefile target 'check-version-map' failed due to missing Chart.yaml file, causing 'repos' and 'image-cozystack' targets to fail with exit code 2.
30-44: Verify precedence order for resource specificationsThe current implementation gives precedence to top-level entries over both requests and limits entries. According to the PR objectives, the precedence should be: top-level, then requests, then limits. However, the current implementation might not follow this order exactly as it merges limits first (lines 30-34), then requests (lines 35-39), and finally top-level entries (lines 40-44).
The final result may be correct because top-level entries overwrite earlier entries, but it would be clearer to process the maps in reverse precedence order: limits, then requests, then top-level.
packages/library/Makefile (1)
16-20: Confirm version map diff path.
Thecheck-version-maptarget runsgit diff --exit-code -- versions_mapfrom withinpackages/library, which matchespackages/library/versions_map. Ensure yourgen_versions_map.shwrites there and that you haven’t relocated or renamed the file.🧰 Tools
🪛 GitHub Actions: Pull Request
[error] 20-20: Makefile target 'check-version-map' failed due to missing Chart.yaml file and versions_map diff mismatch.
c19f470 to
4d25250
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
hack/package_chart.sh (2)
15-21: Make working-directory check more flexible
The script currently insists on running from./packagesby testingbasename $PWD, which can break in CI or when invoked via absolute paths. Consider computing the script’s own directory (e.g.,SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)") and resolving paths relative to that. This decouples the script from the user’s cwd.
23-30: Use numeric comparison and fixechousage
Shell built-ins support arithmetic tests; it’s clearer to writeif [ "$#" -ne 3 ] && [ "$#" -ne 4 ]. Also avoid mixing strings and arrays in a singleechocall (SC2145). For example:-if [ "x$#" != "x3" ] && [ "x$#" != "x4" ] +if [ "$#" -ne 3 ] && [ "$#" -ne 4 ] ... - echo "Got $#: $@" >&2 + echo "Got $#:" "$@" >&2🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 26-26: Argument mixes string and array. Use * or separate argument.
(SC2145)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
Makefile(1 hunks)hack/package_chart.sh(1 hunks)packages/apps/Makefile(1 hunks)packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/charts/cozy-lib(1 hunks)packages/apps/clickhouse/templates/clickhouse.yaml(1 hunks)packages/apps/versions_map(1 hunks)packages/extra/Makefile(1 hunks)packages/library/Makefile(1 hunks)packages/library/cozy-lib/.helmignore(1 hunks)packages/library/cozy-lib/Chart.yaml(1 hunks)packages/library/cozy-lib/Makefile(1 hunks)packages/library/cozy-lib/README.md(1 hunks)packages/library/cozy-lib/templates/_resourcepresets.tpl(1 hunks)packages/library/cozy-lib/templates/_resources.tpl(1 hunks)packages/library/cozy-lib/values.schema.json(1 hunks)packages/library/cozy-lib/values.yaml(1 hunks)packages/library/versions_map(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/apps/versions_map
- packages/apps/clickhouse/charts/cozy-lib
- packages/library/cozy-lib/README.md
- packages/library/cozy-lib/values.schema.json
- packages/library/versions_map
- packages/apps/clickhouse/Chart.yaml
- Makefile
- packages/library/cozy-lib/templates/_resourcepresets.tpl
- packages/library/cozy-lib/.helmignore
- packages/library/cozy-lib/Makefile
- packages/library/cozy-lib/Chart.yaml
- packages/apps/clickhouse/templates/clickhouse.yaml
- packages/library/cozy-lib/templates/_resources.tpl
- packages/apps/Makefile
- packages/library/Makefile
- packages/library/cozy-lib/values.yaml
🧰 Additional context used
🪛 GitHub Actions: Pull Request
packages/extra/Makefile
[error] 20-20: Target 'check-version-map' failed with error
[error] 32-32: Target 'repos' failed due to failure in sub-make in packages/library
[error] 26-26: Target 'image-cozystack' failed due to failure in sub-make in packages/core/installer
[error] 27-27: Top-level 'build' target failed due to failure in sub-make in packages/core/installer
🪛 Shellcheck (0.10.0)
hack/package_chart.sh
[error] 26-26: Argument mixes string and array. Use * or separate argument.
(SC2145)
[warning] 43-43: version appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 51-51: Use "${var:?}" to ensure this never expands to / .
(SC2115)
[warning] 54-54: Use "${var:?}" to ensure this never expands to / .
(SC2115)
🔇 Additional comments (1)
packages/extra/Makefile (1)
1-2: Output and temp-dir setup is consistent
Using a repo-specificOUT=../_out/repos/extraand creating a fresh temp directory withmktemp -daligns with the pattern in other packages, ensuring isolation and no leftover artifacts.
4d25250 to
10afea1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
hack/package_chart.sh (5)
1-3: Consider stricter shell settings and portable interpreter.To catch unset variables and pipeline failures, add
set -uandset -o pipefail. For better portability, use#!/usr/bin/env sh(orbashif you rely on bash features).Possible diff:
-#!/bin/sh -set -e +#!/usr/bin/env sh +set -euo pipefail
5-13: Simplify and maintain the usage message.The series of
printfcalls can be replaced with a here-document for readability:-usage() { - printf "%s\n" "Usage:" >&2 ; - printf -- "%s\n" '---' >&2 ; - printf "%s %s\n" "$0" "INPUT_DIR OUTPUT_DIR TMP_DIR [DEPENDENCY_DIR]" >&2 ; - printf -- "%s\n" '---' >&2 ; - printf "%s\n" "Takes a helm repository from INPUT_DIR, with an optional library repository in" >&2 ; - printf "%s\n" "DEPENDENCY_DIR, prepares a view of the git archive at select points in history" >&2 ; - printf "%s\n" "in TMP_DIR and packages helm charts, outputting the tarballs to OUTPUT_DIR" >&2 ; -} +usage() { + cat << EOF >&2 +Usage: +--- +$0 INPUT_DIR OUTPUT_DIR TMP_DIR [DEPENDENCY_DIR] +--- +Takes a helm repository from INPUT_DIR, with an optional library repository in DEPENDENCY_DIR, +prepares a view of the git archive at select points in history in TMP_DIR, +and packages helm charts, outputting the tarballs to OUTPUT_DIR. +EOF +}
15-21: Quote$PWDin the directory check.The current check may break if the directory name contains spaces or glob chars. Change to:
-if [ "x$(basename $PWD)" != "xpackages" ] +if [ "x$(basename "$PWD")" != "xpackages" ]
32-39: Validate input and optional dependency directories.Before proceeding, verify that
INPUT_DIR(and if provided,DEPENDENCY_DIR) exist and contain the expectedversions_map. This will surface missing or misnamed directories early.
50-50: Check extracted chart path before packaging.Ensure the directory exists and is not empty before calling
helm package, to provide a clearer error message if extraction failed:if [ ! -d "${tmp_dir}/${input_dir}/${package}" ] || [ -z "$(ls -A "${tmp_dir}/${input_dir}/${package}")" ]; then echo "Error: Chart directory '${tmp_dir}/${input_dir}/${package}' is missing or empty" >&2 exit 1 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
Makefile(1 hunks)hack/package_chart.sh(1 hunks)packages/apps/Makefile(1 hunks)packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/charts/cozy-lib(1 hunks)packages/apps/clickhouse/templates/clickhouse.yaml(1 hunks)packages/apps/versions_map(1 hunks)packages/extra/Makefile(1 hunks)packages/library/Makefile(1 hunks)packages/library/cozy-lib/.helmignore(1 hunks)packages/library/cozy-lib/Chart.yaml(1 hunks)packages/library/cozy-lib/Makefile(1 hunks)packages/library/cozy-lib/README.md(1 hunks)packages/library/cozy-lib/templates/_resourcepresets.tpl(1 hunks)packages/library/cozy-lib/templates/_resources.tpl(1 hunks)packages/library/cozy-lib/values.schema.json(1 hunks)packages/library/cozy-lib/values.yaml(1 hunks)packages/library/versions_map(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/library/cozy-lib/values.yaml
- packages/library/cozy-lib/README.md
- packages/apps/clickhouse/charts/cozy-lib
- packages/apps/versions_map
- packages/library/versions_map
- packages/library/cozy-lib/values.schema.json
- packages/apps/clickhouse/Chart.yaml
- Makefile
- packages/library/cozy-lib/Chart.yaml
- packages/library/cozy-lib/templates/_resources.tpl
- packages/apps/Makefile
- packages/library/cozy-lib/.helmignore
- packages/extra/Makefile
- packages/apps/clickhouse/templates/clickhouse.yaml
- packages/library/cozy-lib/templates/_resourcepresets.tpl
- packages/library/cozy-lib/Makefile
- packages/library/Makefile
🧰 Additional context used
🪛 Shellcheck (0.10.0)
hack/package_chart.sh
[error] 26-26: Argument mixes string and array. Use * or separate argument.
(SC2145)
🔇 Additional comments (2)
hack/package_chart.sh (2)
51-55: Cleanup uses safe parameter expansion.Great use of
${var:?}to prevent unintended deletions. This guarantees neithertmp_dir,input_dir,package, nordependency_dirare empty when cleaning up.
56-57: Confirmversions_mapsource file is quoted.The redirection
done < "${input_dir}/versions_map"is correctly quoted, preventing word-splitting. No changes needed here.
abf9652 to
1474bea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
Makefile(1 hunks)hack/gen_versions_map.sh(1 hunks)hack/package_chart.sh(1 hunks)packages/apps/Makefile(1 hunks)packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/charts/cozy-lib(1 hunks)packages/apps/clickhouse/templates/clickhouse.yaml(1 hunks)packages/apps/versions_map(1 hunks)packages/extra/Makefile(1 hunks)packages/library/Makefile(1 hunks)packages/library/cozy-lib/.helmignore(1 hunks)packages/library/cozy-lib/Chart.yaml(1 hunks)packages/library/cozy-lib/Makefile(1 hunks)packages/library/cozy-lib/README.md(1 hunks)packages/library/cozy-lib/templates/_resourcepresets.tpl(1 hunks)packages/library/cozy-lib/templates/_resources.tpl(1 hunks)packages/library/cozy-lib/values.schema.json(1 hunks)packages/library/cozy-lib/values.yaml(1 hunks)packages/library/versions_map(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/library/cozy-lib/Chart.yaml
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/apps/versions_map
- packages/apps/clickhouse/charts/cozy-lib
- packages/library/cozy-lib/values.yaml
- packages/library/cozy-lib/README.md
- packages/library/cozy-lib/values.schema.json
- packages/library/versions_map
- packages/library/cozy-lib/templates/_resourcepresets.tpl
- packages/apps/clickhouse/Chart.yaml
- packages/apps/clickhouse/templates/clickhouse.yaml
- packages/extra/Makefile
- packages/library/cozy-lib/.helmignore
- packages/library/cozy-lib/Makefile
- packages/library/cozy-lib/templates/_resources.tpl
- packages/library/Makefile
- packages/apps/Makefile
- hack/package_chart.sh
- Makefile
🧰 Additional context used
🪛 Shellcheck (0.10.0)
hack/gen_versions_map.sh
[warning] 19-19: In POSIX sh, echo flags are undefined.
(SC3037)
1474bea to
860d18c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
hack/gen_versions_map.sh (1)
19-21: Use printf for POSIX compliance and quote the temp file path
Replaceecho -n(undefined in pure POSIXsh) withprintf '%s'to reliably suppress the newline, and quote the temp‐file variable in the redirection to safely handle paths with spaces.-miss_map=$(mktemp) -trap 'rm -f "$miss_map"' EXIT -echo -n "$new_map" | awk 'NR==FNR { nm[$1 " " $2] = $3; next } { if (!($1 " " $2 in nm)) print $1, $2, $3}' - "$file" > $miss_map +miss_map=$(mktemp) +trap 'rm -f "$miss_map"' EXIT +printf '%s' "$new_map" | awk 'NR==FNR { nm[$1 " " $2] = $3; next } { if (!($1 " " $2 in nm)) print $1, $2, $3}' - "$file" > "$miss_map"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 21-21: In POSIX sh, echo flags are undefined.
(SC3037)
🧹 Nitpick comments (1)
hack/gen_versions_map.sh (1)
27-61: Quote the temp file in the while‐loop redirection
The input redirectiondone < $miss_mapshould be quoted to prevent word splitting if the temp‐file path contains whitespace.- done < $miss_map + done < "$miss_map"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
Makefile(1 hunks)hack/gen_versions_map.sh(2 hunks)hack/package_chart.sh(1 hunks)packages/apps/Makefile(1 hunks)packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/charts/cozy-lib(1 hunks)packages/apps/clickhouse/templates/clickhouse.yaml(1 hunks)packages/apps/versions_map(1 hunks)packages/extra/Makefile(1 hunks)packages/library/Makefile(1 hunks)packages/library/cozy-lib/.helmignore(1 hunks)packages/library/cozy-lib/Chart.yaml(1 hunks)packages/library/cozy-lib/Makefile(1 hunks)packages/library/cozy-lib/README.md(1 hunks)packages/library/cozy-lib/templates/_resourcepresets.tpl(1 hunks)packages/library/cozy-lib/templates/_resources.tpl(1 hunks)packages/library/cozy-lib/values.schema.json(1 hunks)packages/library/cozy-lib/values.yaml(1 hunks)packages/library/versions_map(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/library/cozy-lib/Chart.yaml
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/library/cozy-lib/README.md
- packages/apps/clickhouse/charts/cozy-lib
- packages/apps/versions_map
- packages/library/cozy-lib/values.yaml
- packages/library/versions_map
- packages/library/cozy-lib/values.schema.json
- packages/library/cozy-lib/Makefile
- Makefile
- packages/library/cozy-lib/templates/_resourcepresets.tpl
- packages/extra/Makefile
- packages/library/cozy-lib/.helmignore
- packages/apps/clickhouse/templates/clickhouse.yaml
- packages/apps/clickhouse/Chart.yaml
- packages/library/cozy-lib/templates/_resources.tpl
- hack/package_chart.sh
- packages/apps/Makefile
- packages/library/Makefile
🧰 Additional context used
🪛 Shellcheck (0.10.0)
hack/gen_versions_map.sh
[warning] 21-21: In POSIX sh, echo flags are undefined.
(SC3037)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
860d18c to
6f702b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
hack/gen_versions_map.sh (1)
61-61: Quote the temp file in input redirection
Avoid unquoted variable expansions in input redirection to prevent word splitting or globbing:-done < $miss_map +done < "$miss_map"hack/package_chart.sh (4)
1-3: Enable strict mode for unset variables
Consider treating unset variables as errors by adding-ualongside-e:-set -e +set -eu
15-21: Quotebasename $PWDand simplify working-directory check
To handle spaces robustly and improve readability, quote the argument tobasenameand compare directly:-if [ "x$(basename $PWD)" != "xpackages" ] +if [ "$(basename "$PWD")" != "packages" ]
23-30: Use numeric comparison for argument count
Using arithmetic operators (-ne) clarifies intent and is more idiomatic:-if [ "x$#" != "x3" ] && [ "x$#" != "x4" ] +if [ "$#" -ne 3 ] && [ "$#" -ne 4 ]
25-27: Avoid mixing string and array in echo
ShellCheck SC2145 flags mixing a format string with$@. You can consolidate or use$*:-echo "Got $# arguments:" "$@" >&2 +echo "Got $# arguments: $*" >&2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
Makefile(1 hunks)hack/gen_versions_map.sh(2 hunks)hack/package_chart.sh(1 hunks)packages/apps/Makefile(1 hunks)packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/charts/cozy-lib(1 hunks)packages/apps/clickhouse/templates/clickhouse.yaml(1 hunks)packages/apps/versions_map(1 hunks)packages/extra/Makefile(1 hunks)packages/library/Makefile(1 hunks)packages/library/cozy-lib/.helmignore(1 hunks)packages/library/cozy-lib/Chart.yaml(1 hunks)packages/library/cozy-lib/Makefile(1 hunks)packages/library/cozy-lib/README.md(1 hunks)packages/library/cozy-lib/templates/_resourcepresets.tpl(1 hunks)packages/library/cozy-lib/templates/_resources.tpl(1 hunks)packages/library/cozy-lib/values.schema.json(1 hunks)packages/library/cozy-lib/values.yaml(1 hunks)packages/library/versions_map(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/library/cozy-lib/Chart.yaml
- packages/library/cozy-lib/templates/_resources.tpl
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/library/cozy-lib/values.yaml
- packages/library/versions_map
- packages/library/cozy-lib/README.md
- packages/apps/versions_map
- packages/apps/clickhouse/charts/cozy-lib
- packages/library/cozy-lib/values.schema.json
- packages/apps/clickhouse/Chart.yaml
- packages/extra/Makefile
- packages/library/cozy-lib/Makefile
- packages/apps/clickhouse/templates/clickhouse.yaml
- packages/library/cozy-lib/templates/_resourcepresets.tpl
- packages/library/cozy-lib/.helmignore
- Makefile
- packages/library/Makefile
- packages/apps/Makefile
🧰 Additional context used
🧠 Learnings (1)
hack/package_chart.sh (1)
Learnt from: lllamnyp
PR: cozystack/cozystack#935
File: hack/package_chart.sh:43-49
Timestamp: 2025-05-23T10:57:31.683Z
Learning: When using the `read` command with multiple variables in shell scripts, setting `IFS=` (empty) will cause the entire line to be read into the first variable. To prevent backslash interpretation while maintaining normal field splitting, use `read -r` without changing IFS.
🪛 Shellcheck (0.10.0)
hack/gen_versions_map.sh
[warning] 21-21: In POSIX sh, echo flags are undefined.
(SC3037)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (1)
hack/package_chart.sh (1)
45-57: Packaging loop logic looks solid
The loop correctly handles optional dependencies, safely archives charts at specific commits, uses parameter expansion guards forrm -rf, and produces the Helm repo index. Nicely done.
This patch introduces reusable library charts that provide backward-compatibility for users that specify their resources as explicit requests and limits for cpu, however this input is processed so that limits are set equal to requests except for CPU which only gets requests. Users can now embrace the new form by directly specifying resources in the first level of nesting (e.g. resources.cpu=100m instead of .resources.requests.cpu=100m). The order of precedence is top-level, then requests, then limits, ensuring that nothing will break in terms of scheduling, however workloads that specified limits much higher than requests might get a performance hit, now that they cannot use all this excess capacity. This should only affect memory-hungry workloads in low-contention environments. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
6f702b5 to
9592f7f
Compare
This PR includes fixes and updates for cozystack dashboard: ### [fix client rate limiter](cozystack/kubeapps@b1467ce) fixes the error `client rate limiter Wait returned an error: context canceled` The QPS and Burst options were set after the kubernetes client initalized and had no effect The limits are also increased fivefold: ```diff - - --kube-api-qps=50.0 - - --kube-api-burst=100 + - --kube-api-qps=250.0 + - --kube-api-burst=500 ``` ### [fix relative urls](cozystack/kubeapps@e2153e2) Fixes regression introduced in #935 which suddenly removed previus workaround #102 Now the proper fix prepared. Related to upstream issue vmware-tanzu/kubeapps#7740 ### [remove version selector](cozystack/kubeapps@f412a6a) from both package insallation page and upgrading page <img width="505" alt="Screenshot 2025-06-10 at 1 47 10" src="https://github.com/user-attachments/assets/36068264-2878-4b82-a159-6c911f1c1eef" /> now it always will default to the latest package version ### [always fetch details from the latest version](cozystack/kubeapps@741a7dd) If old package version installed it will display information from the latest package in repository. This and previus fix actually remove the need for having versions_map logic and pack multiple charts for the release. But informs user about newer versions and allows to perform upgrade on demand in specific time: <img width="423" alt="Screenshot 2025-06-10 at 1 52 53" src="https://github.com/user-attachments/assets/dd571c9f-c2bc-403f-9aa0-3d8853600241" /> ### [Remove plugin name from header]cozystack/kubeapps@ffc0b02 We always use flux though <img width="386" alt="Screenshot 2025-06-10 at 1 55 39" src="https://github.com/user-attachments/assets/df6f52b5-82ab-4e7a-a973-2a82eb38ebfb" /> ### [Fix switching context from app view](cozystack/kubeapps@d89e721) Fixes the error message while swtiching tenant from the application view ``` An error occurred while fetching the application: Unable to get installed package. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added new configuration options for API request rate limits in the dashboard settings. - **Style** - Updated dashboard appearance to hide version information and specific label elements. - **Chores** - Updated internal references to the latest version of the dashboard source code. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This PR includes fixes and updates for cozystack dashboard: ### [fix client rate limiter](cozystack/kubeapps@b1467ce) fixes the error `client rate limiter Wait returned an error: context canceled` The QPS and Burst options were set after the kubernetes client initalized and had no effect The limits are also increased fivefold: ```diff - - --kube-api-qps=50.0 - - --kube-api-burst=100 + - --kube-api-qps=250.0 + - --kube-api-burst=500 ``` ### [fix relative urls](cozystack/kubeapps@e2153e2) Fixes regression introduced in #935 which suddenly removed previus workaround #102 Now the proper fix prepared. Related to upstream issue vmware-tanzu/kubeapps#7740 ### [remove version selector](cozystack/kubeapps@f412a6a) from both package insallation page and upgrading page <img width="505" alt="Screenshot 2025-06-10 at 1 47 10" src="https://github.com/user-attachments/assets/36068264-2878-4b82-a159-6c911f1c1eef" /> now it always will default to the latest package version ### [always fetch details from the latest version](cozystack/kubeapps@741a7dd) If old package version installed it will display information from the latest package in repository. This and previus fix actually remove the need for having versions_map logic and pack multiple charts for the release. But informs user about newer versions and allows to perform upgrade on demand in specific time: <img width="423" alt="Screenshot 2025-06-10 at 1 52 53" src="https://github.com/user-attachments/assets/dd571c9f-c2bc-403f-9aa0-3d8853600241" /> ### [Remove plugin name from header]cozystack/kubeapps@ffc0b02 We always use flux though <img width="386" alt="Screenshot 2025-06-10 at 1 55 39" src="https://github.com/user-attachments/assets/df6f52b5-82ab-4e7a-a973-2a82eb38ebfb" /> ### [Fix switching context from app view](cozystack/kubeapps@d89e721) Fixes the error message while swtiching tenant from the application view ``` An error occurred while fetching the application: Unable to get installed package. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added new configuration options for API request rate limits in the dashboard settings. - **Style** - Updated dashboard appearance to hide version information and specific label elements. - **Chores** - Updated internal references to the latest version of the dashboard source code. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This patch introduces reusable library charts that provide backward-compatibility for users that specify their resources as explicit requests and limits for cpu, however this input is processed so that limits are set equal to requests except for CPU which only gets requests. Users can now embrace the new form by directly specifying resources in the first level of nesting (e.g. resources.cpu=100m instead of .resources.requests.cpu=100m). The order of precedence is top-level, then requests, then limits, ensuring that nothing will break in terms of scheduling, however workloads that specified limits much higher than requests might get a performance hit, now that they cannot use all this excess capacity. This should only affect memory-hungry workloads in low-contention environments.
Summary by CodeRabbit
New Features
Bug Fixes
Chores