build(helm): Set up Spider Huntsman Helm chart scaffolding and linting. - #383
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Spider Helm chart scaffold, provisions chart-testing and Helm tooling, and integrates Helm linting into Task and CI development workflows with pipeline failure propagation. ChangesHelm linting integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LintTask
participant HelmTask
participant Toolchain
participant ChartTesting
LintTask->>HelmTask: run check-helm or fix-helm
HelmTask->>Toolchain: activate Helm and the lint virtual environment
HelmTask->>ChartTesting: run ct lint for tools/deployment
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
…dalone. `ct lint` requires a values.yaml in every chart, so PR y-scope#383 could not pass its own `task lint:helm` without one. Add the generic scaffold keys (nameOverride/fullnameOverride); the storage/mariadb values are added in y-scope#384.
20001020ycx
changed the title
20001020ycx
changed the title
|
|
||
| - task: "cmake-check" | ||
| - task: "cpp-check" | ||
| - task: "helm-check" |
There was a problem hiding this comment.
My bad. xxx-check is an obsolete convention. The latest convention is to use check-xxx. For example check-rust. I am ok with both in this PR. @LinZhihao-723 Shall we create a cleanup PR after this one?
There was a problem hiding this comment.
okay, thanks for the catch, good to know
There was a problem hiding this comment.
I actually prefer check-xxx, as we should always use the latest convention when both exist.
fyi, Rust linting is called fix-rust and check-rust.
| *~ | ||
| # Various IDEs | ||
| *.tmproj | ||
| .idea/ |
There was a problem hiding this comment.
For RustRover, we probably need to ignore target/.
There was a problem hiding this comment.
Actually, just dived a bit more on the .helmignore: this ignore file is controlling what files are put into the .tgz during helm package, the .tgz file is the artifact distributed to k8s user.
So, helm package only reads the chart directory and re-bundles its own files into a tarball — it compiles nothing and creates no target/ and no build/. So target/ is not, and never will be, a product of helm package.
I will just use the version inherited from the CLP package for consistency then.
…nting. Adds the Spider Helm chart skeleton (Chart.yaml + .helmignore) and wires CLP-style chart linting: a `helm` toolchain (chart-testing + Helm), a `lint:helm` task running `ct lint`, and `yamale` for Chart.yaml schema validation. The chart's templates and values follow in a separate PR.
…dalone. `ct lint` requires a values.yaml in every chart, so PR y-scope#383 could not pass its own `task lint:helm` without one. Add the generic scaffold keys (nameOverride/fullnameOverride); the storage/mariadb values are added in y-scope#384.
6b95fed to
f9b5fc8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
taskfiles/toolchains.yaml (1)
84-118: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftConsider adding checksum validation for the Helm toolchain.
The Rust task includes full checksum validation (
utils:checksum:validate/utils:checksum:compute) to detect stale or corrupted installations, but the Helm task only usesrun: "once". If the Helm toolchain installation is interrupted,run: "once"won't re-run it in the same session, requiring a manualrm -rfofG_HELM_TOOLCHAIN_DIR.This can be deferred to a follow-up PR, but mirroring the Rust task's checksum pattern would improve reliability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@taskfiles/toolchains.yaml` around lines 84 - 118, Helm installation lacks checksum-based validation and can remain incomplete after an interrupted run. Update the helm task’s run logic to mirror the Rust task’s utils:checksum:validate/utils:checksum:compute pattern, validating the installed G_HELM_TOOLCHAIN_DIR and re-running installation when the checksum is missing or stale, while preserving the existing download and environment-file setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@taskfiles/toolchains.yaml`:
- Around line 110-111: Pin the Helm installer download in the relevant toolchain
command to the immutable tagged release URL for v4.2.3 instead of the mutable
main branch, while preserving the existing curl options and installer arguments.
---
Nitpick comments:
In `@taskfiles/toolchains.yaml`:
- Around line 84-118: Helm installation lacks checksum-based validation and can
remain incomplete after an interrupted run. Update the helm task’s run logic to
mirror the Rust task’s utils:checksum:validate/utils:checksum:compute pattern,
validating the installed G_HELM_TOOLCHAIN_DIR and re-running installation when
the checksum is missing or stale, while preserving the existing download and
environment-file setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7e345885-f0dd-4a8d-95f3-896832138264
📒 Files selected for processing (6)
requirements-dev.txttaskfiles/lint.yamltaskfiles/toolchains.yamltools/deployment/spider-helm/.helmignoretools/deployment/spider-helm/Chart.yamltools/deployment/spider-helm/values.yaml
20001020ycx
changed the title
| @@ -0,0 +1,2 @@ | |||
| nameOverride: "" | |||
| fullnameOverride: "" | |||
There was a problem hiding this comment.
mind explaining what those are and how they are used?
There was a problem hiding this comment.
The values.yaml here is only a placeholder so ct lint has a values file to validate against — linting fails if the file is missing, but its content is irrelevant.
nameOverride/fullnameOverride aren't required for linting and nothing in the chart references them yet, so I've dropped them and left the file empty. I will add the actual content in the follow-up PR.
LOL, this was the initial one that ai suggested. Sure, I will just add Huntsman for clarity then |
20001020ycx
changed the title
…ty for `ct lint`.
…lint task ordering.
… `origin/main` ref.
…p its attributes per the Taskfile guide.
| steps: | ||
| - uses: "actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd" # v6.0.2 | ||
| with: | ||
| submodules: "recursive" |
There was a problem hiding this comment.
sorry for missing this -
do we need fetch-depth: 0 for helm check to work?
There was a problem hiding this comment.
I think I know what you meant, I believe ct -all shall solve the problem.
I'm assuming you're referring to this error we hit when the workflow ran without --all:
Error: failed linting charts: failed identifying charts to process: targetBranch 'origin/main' does not exist
--all skips that git-diff with origin/main and lints every chart directly, as you can see, lint-common passes are green now.
There was a problem hiding this comment.
i see. do you know if the version bump check would also need to use git diff?
in fact, the fetch-depth: 0 option is also specified in the official example: https://github.com/helm/chart-testing-action#:~:text=with%3A%0A%20%20%20%20%20%20%20%20%20%20fetch%2Ddepth%3A%200
either way, the CLP repo will not use the --all option because it will integrate the spider helm chart as a subchart. then for consistency i think it's better to follow the same in spider?
…er the check-xxx convention.
…tead of --all for CLP consistency.
4 participants
Description
See PR title.
Note
This PR is part of the ongoing work Spider Huntsman k8s integration. More PRs will be submitted until the Helm chart is complete and fully functional for Spider Huntsman.
Note for reviewer:
The taskfile implementation is a close copy from CLP:
lint:helm←clp/taskfiles/lint.yaml#L59toolchains:helm←clp/taskfiles/toolchains.yaml#L24The only intentional deviations are the task aliases (
helm-check/helm-fix, to match Spider's<lang>-check/<lang>-fixnaming).Checklist
breaking change.
Validation performed
helm lintandyamllint --strictpass on the committed YAML.task lint:helm-checkresolves the fullct lintpipeline and passes.task lint:checkpasses.Summary by CodeRabbit
pipefail.