Conversation
WalkthroughThe PR pins selected umbrella chart dependencies, normalizes dependency versions during Helm builds, adds Chart.yaml-based publish version fallbacks, verifies ChangesHelm Chart Dependency Management and Release Process
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Dispatch as workflow_dispatch
participant Publish as publish-charts.yaml
participant Yq as yq
participant Chart as charts/osac/Chart.yaml
participant Helm as Helm packaging
Dispatch->>Publish: optional component versions
Publish->>Yq: resolve missing versions
Yq->>Chart: read dependency versions
Chart-->>Yq: pinned versions
Yq-->>Publish: resolved versions
Publish->>Chart: rewrite OCI repositories and versions
Publish->>Helm: package umbrella chart
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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 |
|
@eliorerz: This pull request references OSAC-1409 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eliorerz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
d0c9063 to
1a2ebb4
Compare
| echo "Deploying OSAC using Helm..." | ||
| # Chart.yaml has real published versions for the release workflow, but | ||
| # file:// deps require versions to match the submodule Chart.yaml (0.0.0). | ||
| yq -i '(.dependencies[].version) = "0.0.0"' charts/osac/Chart.yaml |
There was a problem hiding this comment.
this needs yq installed in the osac installer dockerfile in release, please create such pr before merging this pr
There was a problem hiding this comment.
OK, Why won't we keep the dockerfile in this repository?
6f9005d to
056dbf9
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: 2
🤖 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 `@docs/releasing.md`:
- Around line 103-104: The docs incorrectly state that workflow_dispatch falls
back to `published-versions.yaml`; update the text to state that missing
component versions are read from `charts/osac/Chart.yaml` instead, and mention
`workflow_dispatch` as the workflow that uses this fallback so operators are
directed to the correct source of truth (`charts/osac/Chart.yaml`) for manual
publishes rather than `published-versions.yaml`.
In `@scripts/setup.sh`:
- Around line 327-330: The script currently mutates the tracked
charts/osac/Chart.yaml in place using "yq -i" and never restores it; change the
flow to operate on a temporary copy instead: copy charts/osac/Chart.yaml to a
temp file, run yq to set .dependencies[].version="0.0.0" against that temp file,
then run helm dependency build pointing at the temp chart directory (or temp
Chart.yaml) and finally delete the temp; ensure functions/commands referenced
(yq usage, charts/osac/Chart.yaml, helm dependency build) are updated so the
repository file is never modified in place.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a77f5584-f862-42c7-b8c0-eaf78adee055
📒 Files selected for processing (6)
.github/workflows/helm-integration.yaml.github/workflows/helm-lint.yaml.github/workflows/publish-charts.yamlcharts/osac/Chart.yamldocs/releasing.mdscripts/setup.sh
| # Chart.yaml has real published versions for the release workflow, but | ||
| # file:// deps require versions to match the submodule Chart.yaml (0.0.0). | ||
| yq -i '(.dependencies[].version) = "0.0.0"' charts/osac/Chart.yaml | ||
| helm dependency build charts/osac/ |
There was a problem hiding this comment.
Major risk: don’t rewrite the release source-of-truth Chart.yaml in place.
Lines 327-329 permanently mutate the tracked charts/osac/Chart.yaml to fake 0.0.0 dependency versions and never restore it. Because this file now drives tag-based umbrella releases, a local ./scripts/setup.sh run leaves the repo dirty with incorrect versions that are easy to commit or tag by accident.
Suggested fix
- # Chart.yaml has real published versions for the release workflow, but
- # file:// deps require versions to match the submodule Chart.yaml (0.0.0).
- yq -i '(.dependencies[].version) = "0.0.0"' charts/osac/Chart.yaml
- helm dependency build charts/osac/
- helm upgrade --install osac charts/osac/ \
+ # Chart.yaml has real published versions for the release workflow, but
+ # file:// deps require versions to match the submodule Chart.yaml (0.0.0).
+ tmp_chart_dir="$(mktemp -d)"
+ trap 'rm -rf "${tmp_chart_dir}"' EXIT
+ cp -R charts/osac/. "${tmp_chart_dir}/"
+ yq -i '(.dependencies[].version) = "0.0.0"' "${tmp_chart_dir}/Chart.yaml"
+ helm dependency build "${tmp_chart_dir}/"
+ helm upgrade --install osac "${tmp_chart_dir}/" \
--namespace "${INSTALLER_NAMESPACE}" \
--values "${VALUES_FILE}" \
--timeout 40m \
--wait🤖 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 `@scripts/setup.sh` around lines 327 - 330, The script currently mutates the
tracked charts/osac/Chart.yaml in place using "yq -i" and never restores it;
change the flow to operate on a temporary copy instead: copy
charts/osac/Chart.yaml to a temp file, run yq to set
.dependencies[].version="0.0.0" against that temp file, then run helm dependency
build pointing at the temp chart directory (or temp Chart.yaml) and finally
delete the temp; ensure functions/commands referenced (yq usage,
charts/osac/Chart.yaml, helm dependency build) are updated so the repository
file is never modified in place.
|
/retest |
056dbf9 to
0702f33
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/releasing.md`:
- Around line 103-104: The current documentation at line 103 incorrectly states
that all `workflow_dispatch` inputs fall back to `charts/osac/Chart.yaml`, but
this is misleading because the `version` input actually falls back to the
selected git ref/tag name instead. Update the documentation to clarify that only
component versions fall back to `charts/osac/Chart.yaml`, while the `version`
input has a different fallback behavior (the git ref/tag name). This distinction
is important for users performing manual releases to understand the actual
behavior.
In `@scripts/setup.sh`:
- Around line 361-364: Add a prerequisite check for the yq command at the
beginning of the setup.sh script, before any cluster modifications are made.
This check should validate that yq is available and provide a clear, actionable
error message if it is not found. Since the script uses yq in the yq -i command
that modifies charts/osac/Chart.yaml, performing this check upfront prevents the
script from failing mid-run after substantial cluster changes have already
occurred, rather than relying solely on the generic "command not found" error
from set -o errexit.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b08b07dd-9cdc-4b09-bd5e-d5c9efff976c
📒 Files selected for processing (6)
.github/workflows/helm-integration.yaml.github/workflows/helm-lint.yaml.github/workflows/publish-charts.yamlcharts/osac/Chart.yamldocs/releasing.mdscripts/setup.sh
|
🔴 CI Triage: Root cause: The PR has a git merge conflict with the main branch in scripts/setup.sh. Explanation: During the initial source cloning step, Prow's Evidence: Suggestion: Rebase the PR on top of the latest main branch and resolve the merge conflicts in scripts/setup.sh. Prow job | Build For deeper investigation, use the |
0702f33 to
ee05ef7
Compare
ee05ef7 to
881dda3
Compare
When triggered by tag push, the publish workflow now reads dependency versions from charts/osac/Chart.yaml instead of defaulting all components to the umbrella version. This allows tag-based releases to work correctly when component charts have different versions. All workflow_dispatch inputs are now optional, falling back to Chart.yaml values when omitted. Chart.yaml dependency versions updated from 0.0.0 placeholders to current published versions.
881dda3 to
36eb4e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/publish-charts.yaml (1)
89-129: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDownstream of unpinned Chart.yaml deps.
bmf_crds_ver,bmf_ver, andui_verfall back to reading.versionfromcharts/osac/Chart.yaml, which currently still holds">=0.0.0"for these three dependencies (see comment oncharts/osac/Chart.yaml). The semver validation loop at Line 122 will reject these and abort the release. Either pin all seven deps inChart.yaml, or exclude disabled/optional components (bmf, ui) from this unconditional resolve+validate flow.🤖 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 @.github/workflows/publish-charts.yaml around lines 89 - 129, The version resolution in the publish-charts workflow is still unconditionally reading and validating optional component versions from Chart.yaml, which causes the release to fail when those deps are left as >=0.0.0. Update the version handling in the workflow step that sets crds_ver, operator_ver, service_ver, aap_ver, bmf_crds_ver, bmf_ver, and ui_ver so that either all Chart.yaml dependencies are pinned to real semver values or the disabled/optional components (especially bare-metal-fulfillment-operator-crds, bare-metal-fulfillment-operator, and osac-ui) are skipped from the fallback read and semver validation path.
🤖 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 @.github/workflows/helm-integration.yaml:
- Around line 40-45: The chart dependency version rewrite is duplicated in this
workflow and the same logic also exists in helm-lint.yaml and scripts/setup.sh.
Extract the shared `yq`-based pinning behavior into a single helper such as
`scripts/pin-chart-deps-to-local.sh`, then update the workflow step that runs
`helm dependency build` to call that helper instead of inlining the rewrite.
Make sure the shared script preserves the current `Chart.yaml` adjustment for
`charts/osac/Chart.yaml` so all call sites use one source of truth.
In @.github/workflows/publish-charts.yaml:
- Around line 212-224: The Create GitHub Release step in publish-charts is
currently gated with always(), which allows it to run after earlier failures;
change the step condition to require overall success so it only executes when
the publish flow has completed successfully. Update the if expression on the
Create GitHub Release step to use success() instead of always(), keeping the
existing versions step check intact.
- Around line 248-254: The release step in publish-charts.yaml is interpolating
github.repository directly inside the run script; move that value into env
variables like VERSION is already done, then reference the env variable in both
gh release create and gh release edit inside the release creation step. Keep the
fix local to the step that uses gh release create/edit so the shell script no
longer contains inline ${{ github.repository }} expansion.
In `@charts/osac/Chart.yaml`:
- Around line 24-37: The remaining dependencies in Chart.yaml are still using
open-ended version constraints, which will fail the release job’s strict semver
validation. Update the version fields for bare-metal-fulfillment-operator-crds,
bare-metal-fulfillment-operator, and osac-ui to pinned release versions that
match the published charts, keeping the existing aliases and conditions intact.
Use the dependency entries in the osac chart manifest to locate and adjust these
values before publish-charts.yaml runs.
---
Outside diff comments:
In @.github/workflows/publish-charts.yaml:
- Around line 89-129: The version resolution in the publish-charts workflow is
still unconditionally reading and validating optional component versions from
Chart.yaml, which causes the release to fail when those deps are left as
>=0.0.0. Update the version handling in the workflow step that sets crds_ver,
operator_ver, service_ver, aap_ver, bmf_crds_ver, bmf_ver, and ui_ver so that
either all Chart.yaml dependencies are pinned to real semver values or the
disabled/optional components (especially bare-metal-fulfillment-operator-crds,
bare-metal-fulfillment-operator, and osac-ui) are skipped from the fallback read
and semver validation path.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 49d126dd-01c9-4afd-87e6-d3fd5d6150f8
📒 Files selected for processing (6)
.github/workflows/helm-integration.yaml.github/workflows/helm-lint.yaml.github/workflows/publish-charts.yamlcharts/osac/Chart.yamldocs/releasing.mdscripts/setup.sh
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/publish-charts.yaml (1)
89-129: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDownstream of unpinned Chart.yaml deps.
bmf_crds_ver,bmf_ver, andui_verfall back to reading.versionfromcharts/osac/Chart.yaml, which currently still holds">=0.0.0"for these three dependencies (see comment oncharts/osac/Chart.yaml). The semver validation loop at Line 122 will reject these and abort the release. Either pin all seven deps inChart.yaml, or exclude disabled/optional components (bmf, ui) from this unconditional resolve+validate flow.🤖 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 @.github/workflows/publish-charts.yaml around lines 89 - 129, The version resolution in the publish-charts workflow is still unconditionally reading and validating optional component versions from Chart.yaml, which causes the release to fail when those deps are left as >=0.0.0. Update the version handling in the workflow step that sets crds_ver, operator_ver, service_ver, aap_ver, bmf_crds_ver, bmf_ver, and ui_ver so that either all Chart.yaml dependencies are pinned to real semver values or the disabled/optional components (especially bare-metal-fulfillment-operator-crds, bare-metal-fulfillment-operator, and osac-ui) are skipped from the fallback read and semver validation path.
🤖 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 @.github/workflows/helm-integration.yaml:
- Around line 40-45: The chart dependency version rewrite is duplicated in this
workflow and the same logic also exists in helm-lint.yaml and scripts/setup.sh.
Extract the shared `yq`-based pinning behavior into a single helper such as
`scripts/pin-chart-deps-to-local.sh`, then update the workflow step that runs
`helm dependency build` to call that helper instead of inlining the rewrite.
Make sure the shared script preserves the current `Chart.yaml` adjustment for
`charts/osac/Chart.yaml` so all call sites use one source of truth.
In @.github/workflows/publish-charts.yaml:
- Around line 212-224: The Create GitHub Release step in publish-charts is
currently gated with always(), which allows it to run after earlier failures;
change the step condition to require overall success so it only executes when
the publish flow has completed successfully. Update the if expression on the
Create GitHub Release step to use success() instead of always(), keeping the
existing versions step check intact.
- Around line 248-254: The release step in publish-charts.yaml is interpolating
github.repository directly inside the run script; move that value into env
variables like VERSION is already done, then reference the env variable in both
gh release create and gh release edit inside the release creation step. Keep the
fix local to the step that uses gh release create/edit so the shell script no
longer contains inline ${{ github.repository }} expansion.
In `@charts/osac/Chart.yaml`:
- Around line 24-37: The remaining dependencies in Chart.yaml are still using
open-ended version constraints, which will fail the release job’s strict semver
validation. Update the version fields for bare-metal-fulfillment-operator-crds,
bare-metal-fulfillment-operator, and osac-ui to pinned release versions that
match the published charts, keeping the existing aliases and conditions intact.
Use the dependency entries in the osac chart manifest to locate and adjust these
values before publish-charts.yaml runs.
---
Outside diff comments:
In @.github/workflows/publish-charts.yaml:
- Around line 89-129: The version resolution in the publish-charts workflow is
still unconditionally reading and validating optional component versions from
Chart.yaml, which causes the release to fail when those deps are left as
>=0.0.0. Update the version handling in the workflow step that sets crds_ver,
operator_ver, service_ver, aap_ver, bmf_crds_ver, bmf_ver, and ui_ver so that
either all Chart.yaml dependencies are pinned to real semver values or the
disabled/optional components (especially bare-metal-fulfillment-operator-crds,
bare-metal-fulfillment-operator, and osac-ui) are skipped from the fallback read
and semver validation path.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 49d126dd-01c9-4afd-87e6-d3fd5d6150f8
📒 Files selected for processing (6)
.github/workflows/helm-integration.yaml.github/workflows/helm-lint.yaml.github/workflows/publish-charts.yamlcharts/osac/Chart.yamldocs/releasing.mdscripts/setup.sh
🛑 Comments failed to post (4)
.github/workflows/helm-integration.yaml (1)
40-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated
yqrewrite logic across three files.This exact
yq -i '(.dependencies[].version) = "0.0.0"'+ comment block is repeated verbatim inhelm-lint.yamlandscripts/setup.sh. Any future change to the rewrite logic (e.g. adding a new file:// dependency exception) risks drifting out of sync across the three copies.Consider extracting this into a small shared script (e.g.
scripts/pin-chart-deps-to-local.sh) invoked from all three call sites.🤖 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 @.github/workflows/helm-integration.yaml around lines 40 - 45, The chart dependency version rewrite is duplicated in this workflow and the same logic also exists in helm-lint.yaml and scripts/setup.sh. Extract the shared `yq`-based pinning behavior into a single helper such as `scripts/pin-chart-deps-to-local.sh`, then update the workflow step that runs `helm dependency build` to call that helper instead of inlining the rewrite. Make sure the shared script preserves the current `Chart.yaml` adjustment for `charts/osac/Chart.yaml` so all call sites use one source of truth..github/workflows/publish-charts.yaml (2)
212-224: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail file=".github/workflows/publish-charts.yaml" echo "=== file outline ===" python3 - <<'PY' from pathlib import Path p = Path(".github/workflows/publish-charts.yaml") lines = p.read_text().splitlines() for i,l in enumerate(lines,1): if 180 <= i <= 260: print(f"{i:4d}: {l}") PY echo echo "=== locate jobs and relevant if conditions ===" rg -n "^(jobs:| [A-Za-z0-9_-]+:| if: | - name: )" "$file"Repository: osac-project/osac-installer
Length of output: 3953
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path p = Path(".github/workflows/publish-charts.yaml") lines = p.read_text().splitlines() for i,l in enumerate(lines,1): if 1 <= i <= 180: print(f"{i:4d}: {l}") PYRepository: osac-project/osac-installer
Length of output: 9371
Gate the release step on publish success
always()lets this run after a failed build, package, or push, so the workflow can still create/edit a GitHub Release for a chart that never reached GHCR. Useif: success()here instead.🤖 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 @.github/workflows/publish-charts.yaml around lines 212 - 224, The Create GitHub Release step in publish-charts is currently gated with always(), which allows it to run after earlier failures; change the step condition to require overall success so it only executes when the publish flow has completed successfully. Update the if expression on the Create GitHub Release step to use success() instead of always(), keeping the existing versions step check intact.
248-254: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Prefer env vars over inline
${{ }}expansion inrun:scripts.
${{ github.repository }}is expanded directly into the shell script twice here rather than passed viaenv:, which zizmor flags as a template-injection pattern (expression text is substituted into the script before the shell parses it).github.repositoryitself isn't attacker-controllable, but following the safer pattern (already used forVERSIONin this same step) avoids setting a bad precedent for later edits that might interpolate less-trusted values.🔧 Proposed fix
env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} VERSION: ${{ steps.versions.outputs.version }} + REPO: ${{ github.repository }} ... run: | ... gh release create "v${VERSION}" \ - --repo "${{ github.repository }}" \ + --repo "${REPO}" \ --title "OSAC v${VERSION}" \ --notes-file "$BODY" \ || gh release edit "v${VERSION}" \ - --repo "${{ github.repository }}" \ + --repo "${REPO}" \ --notes-file "$BODY"📝 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.env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} VERSION: ${{ steps.versions.outputs.version }} REPO: ${{ github.repository }} run: | gh release create "v${VERSION}" \ --repo "${REPO}" \ --title "OSAC v${VERSION}" \ --notes-file "$BODY" \ || gh release edit "v${VERSION}" \ --repo "${REPO}" \ --notes-file "$BODY"🧰 Tools
🪛 zizmor (1.26.1)
[warning] 249-249: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 249-249: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/publish-charts.yaml around lines 248 - 254, The release step in publish-charts.yaml is interpolating github.repository directly inside the run script; move that value into env variables like VERSION is already done, then reference the env variable in both gh release create and gh release edit inside the release creation step. Keep the fix local to the step that uses gh release create/edit so the shell script no longer contains inline ${{ github.repository }} expansion.Source: Linters/SAST tools
charts/osac/Chart.yaml (1)
24-37: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== Chart.yaml ==\n' cat -n charts/osac/Chart.yaml | sed -n '1,120p' printf '\n== publish-charts workflow files ==\n' fd -a 'publish-charts.yaml' .github . || true fd -a 'publish-charts.yml' .github . || true printf '\n== Search for version resolution / semver validation ==\n' rg -n --hidden -S 'Resolve versions|Invalid semver|bmf_crds_version|bmf_version|ui_version|Chart\.yaml|>=0\.0\.0' .github charts -g '!**/node_modules/**'Repository: osac-project/osac-installer
Length of output: 5893
🏁 Script executed:
#!/bin/bash set -euo pipefail cat -n .github/workflows/publish-charts.yaml | sed -n '55,140p'Repository: osac-project/osac-installer
Length of output: 4707
Pin the remaining chart versions before release
publish-charts.yamlreadsbare-metal-fulfillment-operator-crds,bare-metal-fulfillment-operator, andosac-uifromcharts/osac/Chart.yamlon tag pushes, then validates every dependency against a strict semver regex. With">=0.0.0"still in this file, the release job will fail on those entries and block chart publishing.🤖 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 `@charts/osac/Chart.yaml` around lines 24 - 37, The remaining dependencies in Chart.yaml are still using open-ended version constraints, which will fail the release job’s strict semver validation. Update the version fields for bare-metal-fulfillment-operator-crds, bare-metal-fulfillment-operator, and osac-ui to pinned release versions that match the published charts, keeping the existing aliases and conditions intact. Use the dependency entries in the osac chart manifest to locate and adjust these values before publish-charts.yaml runs.
|
@eliorerz: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
…read-versions-from-chart-yaml # Conflicts: # .github/workflows/publish-charts.yaml # scripts/setup.sh
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/integration-tests.yml (2)
40-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame Chart.yaml-rewrite snippet is duplicated across
scripts/setup.sh, this workflow, and (per stack context)helm-lint.yaml.Consider extracting the
yq -i '(.dependencies[].version) = "0.0.0"' charts/osac/Chart.yamlpatch into a single shared script (e.g.scripts/patch-chart-deps.sh) invoked from all three call sites. Thebuildvsupdateinconsistency flagged above is exactly the kind of drift this duplication invites.🤖 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 @.github/workflows/integration-tests.yml around lines 40 - 45, Extract the duplicated Chart.yaml dependency-version rewrite into a shared script, such as scripts/patch-chart-deps.sh, and update the setup script plus the integration-test and helm-lint workflows to invoke it. Keep the existing yq behavior centralized and ensure each call site uses the same patch before its Helm dependency operation.
1-45: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winWorkflow lacks an explicit
permissions:block.Static analysis flags default (broad)
GITHUB_TOKENpermissions for this workflow. As per path instructions, CI/CD workflows should follow "Least privilege: minimizeGITHUB_TOKENpermissions." Add an explicit top-level or job-levelpermissions:block scoped to only what's needed (likelycontents: readfor an integration-test job).🤖 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 @.github/workflows/integration-tests.yml around lines 1 - 45, Add an explicit least-privilege permissions block to the integration-test workflow, preferably at the job level for integration-test, granting only contents: read required by checkout and submodule access. Do not grant broader token permissions.Sources: Path instructions, Linters/SAST tools
scripts/setup.sh (1)
31-36: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInitialize
DEPLOY_MODEbefore the nounset check
DEPLOY_MODEis referenced here without a default, soset -o nounsetwill abort./scripts/setup.shfor the documented invocation unless the caller exports it explicitly. Default it up front (for example,DEPLOY_MODE=${DEPLOY_MODE:-"helm"}) or make the script tolerate an unset value.🤖 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 `@scripts/setup.sh` around lines 31 - 36, Initialize DEPLOY_MODE before the Helm-mode check so set -o nounset cannot abort when callers omit it. Use the script’s documented default, preserving explicitly provided values, then keep the existing yq validation in the DEPLOY_MODE conditional unchanged.Source: Coding guidelines
🤖 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.
Outside diff comments:
In @.github/workflows/integration-tests.yml:
- Around line 40-45: Extract the duplicated Chart.yaml dependency-version
rewrite into a shared script, such as scripts/patch-chart-deps.sh, and update
the setup script plus the integration-test and helm-lint workflows to invoke it.
Keep the existing yq behavior centralized and ensure each call site uses the
same patch before its Helm dependency operation.
- Around line 1-45: Add an explicit least-privilege permissions block to the
integration-test workflow, preferably at the job level for integration-test,
granting only contents: read required by checkout and submodule access. Do not
grant broader token permissions.
In `@scripts/setup.sh`:
- Around line 31-36: Initialize DEPLOY_MODE before the Helm-mode check so set -o
nounset cannot abort when callers omit it. Use the script’s documented default,
preserving explicitly provided values, then keep the existing yq validation in
the DEPLOY_MODE conditional unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 091b0f4a-5b7a-4d2e-935b-288a4b938265
📒 Files selected for processing (3)
.github/workflows/integration-tests.yml.github/workflows/publish-charts.yamlscripts/setup.sh
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
3 participants
Summary
charts/osac/Chart.yamlinstead of defaulting all components to the umbrella versionworkflow_dispatchinputs changed from required to optional, falling back to Chart.yaml values when omitted0.0.0placeholders to current published versions (fulfillment-service 0.0.64, osac-operator 0.0.1, osac-operator-crds 0.0.1, osac-aap 0.0.3)Summary by CodeRabbit
New Features
Bug Fixes
yqtool is unavailable.Documentation