feat(helm): Add --clp-package-image CLI arg to setup scripts for local image testing (resolves #2019).#2020
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
WalkthroughAdds common argument parsing and image-handling helpers for Helm deployments; updates three setup scripts to call the parser and include image-related Helm flags (loads local Docker images into kind clusters and passes repository/tag/pullPolicy to Helm). Chart version bumped. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/deployment/package-helm/.set-up-common.sh`:
- Around line 79-86: The parse_common_args function sets CLP_PACKAGE_IMAGE from
the next positional argument but doesn't validate that a value exists; update
the --clp-package-image handling in parse_common_args to check that "$2" is
non-empty and does not start with '-' before assigning to CLP_PACKAGE_IMAGE, and
if the check fails print a clear error like "Missing value for
--clp-package-image" to stderr and exit with a non-zero status so flags like the
next option aren't mis-parsed.
- Around line 67-72: The current split uses the first colon and mis-parses
registry ports and missing tags; replace it with a last-colon split using
repo="${image%:*}" and tag="${image##*:}" and then validate the result: ensure
the image contains a tag by checking tag != repo and that the tag does not
contain a '/' (i.e. the part after the final slash contains the colon),
otherwise exit with an error; update the echo to use the validated ${repo} and
${tag} variables (refs: variables image, repo, tag in the snippet).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
tools/deployment/package-helm/.set-up-common.shtools/deployment/package-helm/set-up-multi-dedicated-test.shtools/deployment/package-helm/set-up-multi-shared-test.shtools/deployment/package-helm/set-up-test.sh
hoophalab
left a comment
hoophalab
left a comment
There was a problem hiding this comment.
I don't use the script but it looks correct.
| apiVersion: "v2" | ||
| name: "clp" | ||
| version: "0.1.4-dev.2" | ||
| version: "0.1.4-dev.3" |
There was a problem hiding this comment.
Probably don't need to bump the version?
There was a problem hiding this comment.
anything inside a Helm chart directory is considered part of the chart, unless we add the files to the https://helm.sh/docs/chart_template_guide/helm_ignore_file/
if the version is not updated, the linter complains:
------------------------------------------------------------------------------------------------------------------------
Charts to be processed:
------------------------------------------------------------------------------------------------------------------------
clp => (version: "0.1.4-dev.3", path: "tools/deployment/package-helm")
------------------------------------------------------------------------------------------------------------------------
Linting chart "clp => (version: \"0.1.4-dev.3\", path: \"tools/deployment/package-helm\")"
Checking chart "clp => (version: \"0.1.4-dev.3\", path: \"tools/deployment/package-helm\")" for a version bump...
Old chart version: 0.1.4-dev.3
New chart version: 0.1.4-dev.3
------------------------------------------------------------------------------------------------------------------------
✖︎ clp => (version: "0.1.4-dev.3", path: "tools/deployment/package-helm") > chart version not ok. Needs a version bump!
------------------------------------------------------------------------------------------------------------------------
Error: failed linting charts: failed processing charts
failed linting charts: failed processing charts
task: Failed to run task "lint:helm": exit status 1
junhaoliao
changed the title
--clp-package-image CLI arg to setup scripts for local image testing (resolves #2019).
2 participants
Description
Add a
--clp-package-imageCLI argument to the Helm chart setup scripts(
set-up-test.sh,set-up-multi-dedicated-test.sh,set-up-multi-shared-test.sh) so that alocally-built Docker image can be loaded into the kind cluster and used for deployment.
When
--clp-package-imageis omitted,CLP_PACKAGE_IMAGEis empty,get_image_helm_argsreturnsnothing, and the scripts behave exactly as before.
Also hardens
.set-up-common.shwith two robustness fixes:parse_common_argsnow validates that--clp-package-imageis followed by a non-flag value(previously,
set -o nounsetwould crash with a confusing "unbound variable" error, or the nextflag would be silently consumed as the image name).
get_image_helm_argsnow uses a regex to splitrepo:tagon the last colon whose right sidecontains no
/(previously,${image%%:*}would mis-parse registry ports likelocalhost:5000/repo:tag, and images without tags were silently accepted).Checklist
breaking change.
Validation performed
1. Deploy with
--clp-package-imageAll 14 pods running with 0 restarts.
2. Deploy without args (backwards compatibility)
All 14 pods running with 0 restarts. Default image pulled from
ghcr.io/y-scope/clp/clp-package.3. Unknown argument
bash tools/deployment/package-helm/set-up-test.sh --bad-arg # → Unknown argument: --bad-arg (exit 1)4. Missing value for
--clp-package-imageTests call
parse_common_argswith the given input and check the exit code and output.--clp-package-image(end of args)Error: '--clp-package-image' requires a value.--clp-package-image --other-flagError: '--clp-package-image' requires a value.--clp-package-image clp-package:dev-testCLP_PACKAGE_IMAGE=clp-package:dev-testCLP_PACKAGE_IMAGE=--bad-argUnknown argument: --bad-arg--clp-package-image --clp-package-imageError: '--clp-package-image' requires a value.6/6 passed.
5. Image reference parsing
Tests call
get_image_helm_args "test-cluster" <input>and check the generated--setflags.clp-package:devclp-packagedevlocalhost:5000/repo:v1localhost:5000/repov1registry.io:5000/org/repo:latestregistry.io:5000/org/repolatestclp-package(no tag)localhost:5000/repo(port but no tag)ghcr.io/y-scope/clp:v0.3.0-rc1ghcr.io/y-scope/clpv0.3.0-rc1""(empty / omitted)7/7 passed.
Summary by CodeRabbit