[build-system] Build repo with library dependencies - #943
[build-system] Build repo with library dependencies#943Andrei Kvapil (kvaps) wants to merge 1 commit into
Conversation
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
WalkthroughA new shell script, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Makefile
participant build-repo.sh
participant Git
participant Helm
User->>Makefile: make repo
Makefile->>build-repo.sh: Execute script
build-repo.sh->>Git: Read versions_map, checkout/extract chart sources
build-repo.sh->>Helm: Package charts
build-repo.sh->>build-repo.sh: Replace symlinks with chart files
build-repo.sh->>Helm: Generate repo index
build-repo.sh->>User: Output packaged charts and index
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
hack/build-repo.sh (3)
25-28: Harden the library-chart loop with safe reads and quoting.Use
read -rto prevent backslash interpretation, and always quote variable expansions:- (set -x; find "$TMP/$chart-$version/charts" -type l -maxdepth 1 -mindepth 1) | awk -F/ '{print $NF}' | while read library_chart; do + (set -x; find "$TMP/$chart-$version/charts" -type l -maxdepth 1 -mindepth 1) \ + | awk -F/ '{print $NF}' \ + | while read -r library_chart; do (set -x; rm -f "$TMP/$chart-$version/charts/$library_chart") (set -x; cd ../library && git archive "$commit" "$library_chart" | tar -xf- -C "$TMP/$chart-$version/charts/") done
2-3: Ensure temporary data is always cleaned up.If the script exits early,
$TMPmay remain. Add a trap afterset -e:set -e +trap 'rm -rf "$TMP"' EXIT
33-33: Prevent word-splitting in the finalhelm packagecall.Unquoted
$(…)can split on whitespace. You can pipefindintoxargsor use-execto safely handle paths:- (set -x; helm package -d "$OUT" $(find . "$TMP" -mindepth 2 -maxdepth 2 -name Chart.yaml | awk 'sub("/Chart.yaml", "")' | sort -V)) + (set -x; find . "$TMP" -mindepth 2 -maxdepth 2 -name Chart.yaml \ + | awk 'sub("/Chart.yaml","")' \ + | sort -V \ + | xargs -I{} helm package -d "$OUT" "{}")🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 33-33: Quote this to prevent word splitting.
(SC2046)
packages/extra/Makefile (1)
2-2: Declare.PHONYtargets for clarity.Marking recipe-only targets as phony prevents conflicts with files of the same name:
.PHONY: repo fix-chartnames gen-versions-map check-version-mappackages/apps/Makefile (1)
2-2: Mark phony targets to avoid filename collisions.Add at the top of the Makefile:
.PHONY: repo fix-chartnames gen-versions-map check-version-map
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
hack/build-repo.sh(1 hunks)packages/apps/Makefile(1 hunks)packages/extra/Makefile(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
hack/build-repo.sh
[warning] 30-30: tmp is referenced but not assigned.
(SC2154)
[warning] 33-33: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Test
- GitHub Check: pre-commit
| (set -x; cd ../library && git archive "$commit" "$library_chart" | tar -xf- -C "$TMP/$chart-$version/charts/") | ||
| done | ||
| fi | ||
| rm -rf "$tmp" |
There was a problem hiding this comment.
Critical cleanup bug: wrong variable name.
The line rm -rf "$tmp" references an undefined $tmp and leaves per-chart directories intact. It should remove the specific extracted chart directory:
- rm -rf "$tmp"
+ rm -rf "$TMP/$chart-$version"📝 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.
| rm -rf "$tmp" | |
| rm -rf "$TMP/$chart-$version" |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 30-30: tmp is referenced but not assigned.
(SC2154)
| helm package -d "$(OUT)" $$(find . $(TMP) -mindepth 2 -maxdepth 2 -name Chart.yaml | awk 'sub("/Chart.yaml", "")' | sort -V) | ||
| cd "$(OUT)" && helm repo index . --url http://cozystack.cozy-system.svc/repos/extra | ||
| rm -rf "$(TMP)" | ||
| ../../hack/build-repo.sh |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a prerequisite to generate versions_map.
Invoking make repo now fails if versions_map isn’t present. Ensure it’s built first:
-repo:
- ../../hack/build-repo.sh
+repo: gen-versions-map
+ ../../hack/build-repo.sh📝 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.
| ../../hack/build-repo.sh | |
| repo: gen-versions-map | |
| ../../hack/build-repo.sh |
| helm package -d "$(OUT)" $$(find . $(TMP) -mindepth 2 -maxdepth 2 -name Chart.yaml | awk 'sub("/Chart.yaml", "")' | sort -V) | ||
| cd "$(OUT)" && helm repo index . --url http://cozystack.cozy-system.svc/repos/apps | ||
| rm -rf "$(TMP)" | ||
| ../../hack/build-repo.sh |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add dependency on gen-versions-map before building the repo.
Without this, build-repo.sh will error if versions_map isn’t generated:
-repo:
- ../../hack/build-repo.sh
+repo: gen-versions-map
+ ../../hack/build-repo.sh|
Suppressed by #935 |
Signed-off-by: Andrei Kvapil kvapss@gmail.com
Summary by CodeRabbit
New Features
Chores