OSAC-1906: Add osac-ui as subchart dependency in umbrella chart - #376
Conversation
|
@eliorerz: This pull request references OSAC-1906 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 |
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (14)
WalkthroughAdds osac-ui as a new component throughout the installer: a new git submodule, CI workflow support for validating and bumping its image and publishing its chart version, a Helm chart dependency with schema and default values, and image-tag sync script updates for the UI component. Changesosac-ui component onboarding
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: 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 |
|
/lgtm |
|
@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. |
728ca16 to
04f62e6
Compare
|
New changes are detected. LGTM label has been removed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/bump-submodules.yaml:
- Line 83: The update_submodule call is still interpolating
steps.find.outputs.ui directly in the workflow command, which triggers
template-injection warnings. Update the workflow step that invokes
update_submodule for base/osac-ui so the resolved SHA is passed through the
step’s env block and referenced from that environment variable instead of raw
${{ }} interpolation, matching the safer pattern used in the surrounding
bump-submodules workflow.
In `@charts/osac/ci/bundled-postgres-values.yaml`:
- Around line 8-19: The bundled Postgres values use
ui.api.fulfillment.certs.caBundle and ui.auth.certs.caBundle, but the ui section
of values.schema.json does not define those nested properties. Update the schema
under ui to include the missing api.fulfillment.certs.caBundle and
auth.certs.caBundle entries so it matches the values used in
bundled-postgres-values.yaml and stays in sync with the ui schema definitions.
In `@charts/osac/values.schema.json`:
- Around line 580-604: The ui schema is missing the nested api and auth fields
that are already consumed by ui.* values, so add matching schema entries under
the ui object for ui.api and ui.auth with their expected nested structures.
Update values.schema.json near the ui definition to include the specific
subproperties used by the existing values files, including
ui.api.fulfillment.url, ui.api.fulfillment.certs.caBundle.configMap,
ui.auth.oidcClientId, and ui.auth.certs.caBundle.configMap, so schema validation
catches typos and unknown keys.
In `@charts/osac/values.yaml`:
- Around line 139-158: The new UI configuration keys in values.yaml are not
represented in values.schema.json, so add matching schema definitions for
ui.api, ui.auth, and ui.log alongside the existing ui.enabled,
ui.externalHostname, and ui.images.ui entries. Update the ui object in
values.schema.json to document and validate the nested fulfillment URL and certs
under ui.api, the oidcClientId and certs under ui.auth, and the level field
under ui.log, keeping the schema aligned with the ui values structure.
🪄 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: 6089af3a-233a-4a7d-b7d5-3d16245b465d
📒 Files selected for processing (14)
.github/workflows/bump-submodules.yaml.github/workflows/publish-charts.yaml.gitmodulesbase/osac-uicharts/osac/Chart.yamlcharts/osac/ci/bundled-postgres-values.yamlcharts/osac/ci/default-values.yamlcharts/osac/ci/full-values.yamlcharts/osac/ci/no-aap-values.yamlcharts/osac/values.schema.jsoncharts/osac/values.yamlscripts/sync-image-tags.shvalues/development/values.yamlvalues/vmaas-ci/values.yaml
| update_submodule base/osac-operator osac-operator "${{ steps.find.outputs.operator }}" | ||
| update_submodule base/osac-fulfillment-service fulfillment-service "${{ steps.find.outputs.fulfillment }}" | ||
| update_submodule base/osac-aap osac-aap "${{ steps.find.outputs.aap }}" | ||
| update_submodule base/osac-ui osac-ui "${{ steps.find.outputs.ui }}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
Consider passing the resolved SHA via env: instead of raw ${{ }} interpolation.
Static analysis flags this as a template-injection pattern. The value here (steps.find.outputs.ui) is a trusted commit SHA from the GitHub API within this job, so exploitability is low today — but it mirrors the same pattern already used at lines 80-82. Passing it through an environment variable removes the whole class of risk if the source of this value ever changes.
🔒️ Defense-in-depth fix
- update_submodule base/osac-ui osac-ui "${{ steps.find.outputs.ui }}"
+ update_submodule base/osac-ui osac-ui "${UI_SHA}"And add to the step's env: block:
- name: Update submodules
id: update
+ env:
+ UI_SHA: ${{ steps.find.outputs.ui }}
run: |📝 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.
| update_submodule base/osac-ui osac-ui "${{ steps.find.outputs.ui }}" | |
| - name: Update submodules | |
| id: update | |
| env: | |
| UI_SHA: ${{ steps.find.outputs.ui }} | |
| run: | | |
| update_submodule base/osac-ui osac-ui "${UI_SHA}" |
🧰 Tools
🪛 zizmor (1.26.1)
[info] 83-83: 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/bump-submodules.yaml at line 83, The update_submodule call
is still interpolating steps.find.outputs.ui directly in the workflow command,
which triggers template-injection warnings. Update the workflow step that
invokes update_submodule for base/osac-ui so the resolved SHA is passed through
the step’s env block and referenced from that environment variable instead of
raw ${{ }} interpolation, matching the safer pattern used in the surrounding
bump-submodules workflow.
Source: Linters/SAST tools
Add the osac-ui web console to the osac-installer umbrella Helm chart: - Add osac-ui git submodule under base/ - Add subchart dependency in Chart.yaml (alias: ui, condition: ui.enabled) - Add ui values to values.yaml, values.schema.json, CI and env values files - Update bump-submodules, publish-charts, and sync-image-tags automation
04f62e6 to
144c74c
Compare
|
/ok-to-test |
3 participants
Summary
osac-uigit submodule underbase/Chart.yaml(alias:ui, condition:ui.enabled)ui:values section tovalues.yaml,values.schema.json, all CI values files, and environment values files (development, vmaas-ci)bump-submodules,publish-charts, andsync-image-tags.shautomation to include osac-uiWhy
The OSAC UI web console needs to be deployed as part of the umbrella Helm chart, following the same pattern used by operator, fulfillment-service, aap, and bmf.
Dependencies
deploy/chart/tocharts/ui/)Test plan
helm dependency build charts/osac/succeedshelm templatewith development values renders UI resourcessync-image-tags.shreportsvmaas-ci osac-ui: OKSummary by CodeRabbit
New Features
Bug Fixes