fix: regenerate Chart.lock, clean up .gitignore, add Chart.lock CI validation#268
fix: regenerate Chart.lock, clean up .gitignore, add Chart.lock CI validation#268
Conversation
The Chart.lock had a placeholder digest hash that caused the chart-releaser CI to fail with 'lock file out of sync' error. Also add developer-portal-fe/charts to .gitignore to match other charts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced three explicit .gitignore entries that targeted specific chart subdirectories with a single glob Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions"
participant CT as "chart-testing Docker"
participant Helm as "Helm (inside CT)"
participant Charts as "charts/*/ Chart dirs"
GH->>Charts: scan charts/*/ for Chart.lock
alt Chart.lock exists
GH->>CT: start container for that chart
CT->>Helm: add required Helm repos (repo add)
CT->>Helm: run helm dependency build in chart dir
Helm-->>CT: return build result
CT-->>GH: report success/failure
else no Chart.lock
GH-->>Charts: skip chart
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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)
Comment |
.gitignore
Outdated
| @@ -5,3 +5,4 @@ charts/api7-dashboard/charts | |||
| charts/devportal/charts | |||
There was a problem hiding this comment.
Is this redundant?
There was a problem hiding this comment.
Yes, charts/devportal/charts is a leftover — there's no devportal chart in the repo anymore (only api7, developer-portal-fe, gateway, ingress-controller). Similarly api7-gateway and api7-dashboard may be stale.
I'll clean up the legacy entries in this PR.
There was a problem hiding this comment.
Can CI be added to detect this issue in advance?
There was a problem hiding this comment.
Good idea. The existing Helm chart CI already runs helm lint and helm install but doesn't verify the lock file. Adding a helm dependency build step to the CI workflow would catch this — it fails when the lock digest doesn't match.
I'll add a validation step to the CI in this PR.
- Replace individual chart/*/charts entries with a single wildcard pattern, removing stale entries (api7-gateway, api7-dashboard, devportal) - Add 'Verify Chart.lock files' CI step that runs helm dependency build on every chart with a Chart.lock, catching digest mismatches before merge Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
nic-6443
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)
39-54: Consider consolidating into a single Docker invocation for efficiency.Spinning up a new Docker container for each chart with a
Chart.lockadds overhead (container startup + repeatedhelm repo addoperations). If multiple charts have lock files, this can noticeably slow CI. A single container running a loop internally would be more efficient.Additionally, quoting
${chart_dir}in the inner shell command would improve robustness against edge cases.♻️ Proposed refactor: single container with proper quoting
- name: Verify Chart.lock files run: | - for chart_dir in charts/*/; do - if [ -f "${chart_dir}Chart.lock" ]; then - echo "Verifying ${chart_dir}Chart.lock..." - docker run --rm --interactive --network host \ - --volume $PWD:/workdir \ - --workdir /workdir \ - quay.io/helmpack/chart-testing:v3.10.1 sh -c " - helm repo add bitnami https://charts.bitnami.com/bitnami && \ - helm repo add apisix https://charts.apiseven.com && \ - helm repo add jaegertracing https://jaegertracing.github.io/helm-charts && \ - helm dependency build ${chart_dir} - " - fi - done + docker run --rm --interactive --network host \ + --volume "$PWD:/workdir" \ + --workdir /workdir \ + quay.io/helmpack/chart-testing:v3.10.1 sh -c ' + helm repo add bitnami https://charts.bitnami.com/bitnami && \ + helm repo add apisix https://charts.apiseven.com && \ + helm repo add jaegertracing https://jaegertracing.github.io/helm-charts && \ + for chart_dir in charts/*/; do + if [ -f "${chart_dir}Chart.lock" ]; then + echo "Verifying ${chart_dir}Chart.lock..." + helm dependency build "${chart_dir}" + fi + done + '🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 39 - 54, The current "Verify Chart.lock files" step spawns a new docker container per chart_dir and re-runs helm repo adds each time; change it to invoke docker run exactly once and perform the for-loop inside that single container (move the for chart_dir in charts/*/ loop into the sh -c command) to avoid repeated container startups and duplicated helm repo add calls, while preserving the existing --volume, --workdir and --network flags; also ensure you quote "${chart_dir}" inside the inner shell command (and in the helm dependency build invocation) to handle paths with spaces.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yaml:
- Around line 39-54: The current "Verify Chart.lock files" step spawns a new
docker container per chart_dir and re-runs helm repo adds each time; change it
to invoke docker run exactly once and perform the for-loop inside that single
container (move the for chart_dir in charts/*/ loop into the sh -c command) to
avoid repeated container startups and duplicated helm repo add calls, while
preserving the existing --volume, --workdir and --network flags; also ensure you
quote "${chart_dir}" inside the inner shell command (and in the helm dependency
build invocation) to handle paths with spaces.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fe0771e-c969-4b5c-b0d5-425f365fda4e
📒 Files selected for processing (2)
.github/workflows/ci.yaml.gitignore
✅ Files skipped from review due to trivial changes (1)
- .gitignore
Move the for-loop inside the Docker container to avoid repeated container
startups and helm repo add calls. Also properly quote ${chart_dir}.
Suggested-by: CodeRabbit
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2 participants
Problem
The Release Charts CI (run #23532815477) failed after merging #266 with:
The
Chart.lockcommitted in #266 had a placeholder digest hash instead of a real one generated byhelm dependency update.Changes
1. Regenerate Chart.lock (commit 1)
Ran
helm dependency update charts/developer-portal-fe/to produce a valid digest.2. Clean up .gitignore + add CI validation (commit 2)
.gitignore: Replaced 4 individualcharts/*/chartsentries (including stale ones likeapi7-gateway,api7-dashboard,devportal) with a single wildcard patterncharts/*/chartsVerify Chart.lock filesstep that runshelm dependency buildon every chart with aChart.lock, catching digest mismatches before mergeSummary by CodeRabbit