OSAC-22: Create OSAC umbrella Helm chart and installer tooling - #98
Conversation
|
@eliorerz: This pull request references OSAC-693 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. This pull request references OSAC-694 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. This pull request references OSAC-695 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. This pull request references OSAC-696 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. |
|
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:
WalkthroughThis PR introduces comprehensive Helm-based deployment infrastructure for OSAC. The umbrella chart orchestrates four local sub-chart dependencies (operator CRDs, operator, fulfillment service, AAP) with reusable template helpers and installation notes. A JSON schema constrains all configurable values, with defaults provided and environment-specific overrides for development, CaaS CI, and VMaaS CI profiles. Conditional Helm hooks validate cluster prerequisites before install/upgrade and run database migrations before upgrades. A new Makefile standardizes Helm operations and setup/teardown commands. The setup script now supports both Helm (default) and legacy Kustomize deployment modes with mode-specific configuration. Two GitHub Actions workflows (helm-lint for PRs, helm-integration for scheduled testing) automate chart validation and integration testing on a Kind cluster. Documentation has been substantially expanded to present Helm as the recommended approach with detailed deployment phases, troubleshooting guides, and profile-specific examples. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 7
🧹 Nitpick comments (2)
.github/workflows/helm-integration.yaml (1)
19-20: ⚡ Quick winUpdating submodules to remote
mainintroduces non-reproducible behavior.Line 20 runs
git submodule update --remote, which pulls the latest commit from each submodule's default branch. This makes the integration test non-reproducible and can introduce unexpected failures if dependencies introduce breaking changes.For a daily integration test that validates against the latest dependency versions, this might be intentional. However, if the goal is reproducible PR validation, consider removing this step to test against the pinned submodule commits.
💡 Alternative approaches
Option 1: Remove for reproducibility (recommended for PR testing)
- - name: Update submodules to latest main - run: git submodule update --remote -Option 2: Keep but document the intent (for daily canary testing)
Add a comment clarifying this is intentional canary testing:- name: Update submodules to latest main + # Intentional: daily test validates against latest upstream changes run: git submodule update --remote🤖 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 19 - 20, The workflow step that runs the command "git submodule update --remote" makes test runs non-reproducible by pulling latest submodule commits; either remove that run step to ensure PR validation uses pinned submodule commits, or replace it with an explicit, documented canary path (e.g., move the "git submodule update --remote" command into a scheduled/daily job or add a comment above the step clarifying it is intentional for canary testing); update the job configuration accordingly so PR checks use the committed submodule state and only the intended scheduled job performs remote updates.charts/osac/Chart.yaml (1)
7-34: 💤 Low valueAll dependencies use version
0.0.0- confirm this is intentional.Lines 9, 13, 17, 21, 25, 29, and 32 specify
version: "0.0.0"for all chart dependencies. While Helm accepts any version string forfile://dependencies, using a placeholder version provides no versioning information and makes it harder to track compatibility.If this is intentional for local development (where
file://paths always reference the current state of submodules), consider documenting this choice. Otherwise, consider using semantic versions that match the actual component chart versions to improve traceability.📋 Optional: Add documentation comment
dependencies: + # Note: Version 0.0.0 is used for all file:// dependencies since they track + # git submodule commits rather than published chart versions - name: osac-operator-crds version: "0.0.0"🤖 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 7 - 34, Chart.yaml currently sets version: "0.0.0" for all dependency entries (names: osac-operator-crds, osac-operator, fulfillment-service, postgres, keycloak, ca, osac-aap); update each dependency to either the correct semantic chart version that matches the referenced component or, if this is intentional for local file:// development, add a short comment in Chart.yaml documenting that 0.0.0 is a deliberate placeholder used for local/file:// dependencies and the reason for it (e.g., submodule-driven local installs) so reviewers and future maintainers know this choice.
🤖 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 48-59: Remove the "|| true" silencing from the "Deploy umbrella
chart" step so helm install failures surface; instead add GitHub Actions'
continue-on-error: true to that step (the step named "Deploy umbrella chart"
that runs the helm install osac ... command) so the step is marked failed but
the job continues to the diagnostics/collection step—update the step metadata to
include continue-on-error: true and delete the trailing "|| true" from the helm
install command.
- Around line 73-83: The idempotency step named "Test idempotency (helm upgrade
with no changes)" currently appends "|| true" to the helm upgrade command which
masks failures; remove the trailing "|| true" from that helm upgrade invocation
so the step fails on upgrade errors, or if you intend it to be non-blocking
replace the suppressed exit with GitHub Actions' native handling by adding
continue-on-error: true to that job/step while keeping the helm upgrade command
unchanged; locate the step by its name and modify the helm upgrade run block
accordingly.
- Around line 32-37: The workflow step "Install cert-manager" uses the /latest/
manifest URL which makes runs non-reproducible; change the install command in
the "Install cert-manager" job to pin a specific cert-manager release (e.g., set
a CERT_MANAGER_VERSION variable and replace
https://github.com/cert-manager/cert-manager/releases/latest/download/cert-manager.yaml
with the corresponding release URL like
.../releases/download/<CERT_MANAGER_VERSION>/cert-manager.yaml), then keep the
kubectl wait lines as-is to wait for deployment/cert-manager,
deployment/cert-manager-webhook, and deployment/cert-manager-cainjector to
become Available.
In `@charts/osac/values.schema.json`:
- Around line 72-76: The schema defines "insecureSkipVerify" with type "string"
and default "true" which is wrong; change the field "insecureSkipVerify" to type
"boolean" and set its default to true (unquoted boolean), and then update any
values files (e.g., values/vmaas-ci.yaml and any other occurrences) to use
true/false booleans rather than quoted strings; follow the pattern used by other
boolean fields like operatorCrds.install, postgres.enabled, and keycloak.enabled
to ensure validation passes.
- Around line 208-218: The dbMigrate schema only defines enabled but the hook
template expects .Values.dbMigrate.image, .Values.dbMigrate.dbUrl and
.Values.dbMigrate.dbUrlFile; update the "dbMigrate" object in values.schema.json
to add an "image" property (type: object) with nested properties "repository"
(string), "tag" (string) and "pullPolicy" (string) and sensible defaults, and
add "dbUrl" and "dbUrlFile" properties (type: string, nullable) with clear
descriptions so the template referenced by
charts/osac/templates/hooks/db-migrate.yaml can be validated and documented.
In `@scripts/setup.sh`:
- Around line 279-283: The call to wait_for_resource uses an unquoted variable
which can suffer word-splitting; update the invocation in the conditional block
that handles kustomize to quote the namespace variable (change wait_for_resource
job/aap-bootstrap condition=complete 2400 ${INSTALLER_NAMESPACE} to use
"${INSTALLER_NAMESPACE}"), ensuring the wait_for_resource invocation and any
other uses of INSTALLER_NAMESPACE in that block are quoted to prevent
word-splitting; refer to the wait_for_resource invocation in the kustomize
branch to locate the change.
- Around line 258-272: The oc apply invocation may perform
word-splitting/globbing because INSTALLER_KUSTOMIZE_OVERLAY is unquoted; update
the command that references overlays/${INSTALLER_KUSTOMIZE_OVERLAY} to quote the
variable (e.g. use "overlays/${INSTALLER_KUSTOMIZE_OVERLAY}" or
overlays/"${INSTALLER_KUSTOMIZE_OVERLAY}") so the INSTALLER_KUSTOMIZE_OVERLAY
value is treated as a single argument and special characters are not expanded.
---
Nitpick comments:
In @.github/workflows/helm-integration.yaml:
- Around line 19-20: The workflow step that runs the command "git submodule
update --remote" makes test runs non-reproducible by pulling latest submodule
commits; either remove that run step to ensure PR validation uses pinned
submodule commits, or replace it with an explicit, documented canary path (e.g.,
move the "git submodule update --remote" command into a scheduled/daily job or
add a comment above the step clarifying it is intentional for canary testing);
update the job configuration accordingly so PR checks use the committed
submodule state and only the intended scheduled job performs remote updates.
In `@charts/osac/Chart.yaml`:
- Around line 7-34: Chart.yaml currently sets version: "0.0.0" for all
dependency entries (names: osac-operator-crds, osac-operator,
fulfillment-service, postgres, keycloak, ca, osac-aap); update each dependency
to either the correct semantic chart version that matches the referenced
component or, if this is intentional for local file:// development, add a short
comment in Chart.yaml documenting that 0.0.0 is a deliberate placeholder used
for local/file:// dependencies and the reason for it (e.g., submodule-driven
local installs) so reviewers and future maintainers know this choice.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d44fe56-f901-4820-b81d-c2767efdbbde
📒 Files selected for processing (14)
.github/workflows/helm-integration.yaml.github/workflows/helm-lint.yamlMakefilecharts/osac/Chart.yamlcharts/osac/templates/NOTES.txtcharts/osac/templates/_helpers.tplcharts/osac/templates/hooks/db-migrate.yamlcharts/osac/templates/hooks/pre-install-validate.yamlcharts/osac/values.schema.jsoncharts/osac/values.yamlscripts/setup.shvalues/caas-ci.yamlvalues/development.yamlvalues/vmaas-ci.yaml
26c3873 to
1b8b210
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
charts/osac/values.schema.json (1)
72-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winType inconsistency:
insecureSkipVerifyshould be boolean, not string.The
insecureSkipVerifyfield is defined astype: "string"with a default of"true"(string), which is inconsistent with standard Kubernetes boolean conventions and other boolean fields in this schema (e.g.,operatorCrds.install,validation.enabled,dbMigrate.enabled).This type inconsistency can cause validation issues and confusion for users expecting boolean behavior.
🔧 Proposed fix
"insecureSkipVerify": { - "type": "string", + "type": "boolean", "description": "Skip TLS verification for AAP", - "default": "true" + "default": true },Note: This change requires updating the corresponding values files:
values/caas-ci.yamlline 13values/development.yamlline 14values/vmaas-ci.yamlline 13Change from
insecureSkipVerify: "true"toinsecureSkipVerify: true(unquoted boolean).🤖 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/values.schema.json` around lines 72 - 76, The schema field insecureSkipVerify is incorrectly declared as type "string" with default "true"; change its schema entry in values.schema.json to type "boolean" and default true (unquoted) and update the corresponding values files that set insecureSkipVerify to use the boolean true instead of the string "true" so validation aligns with other boolean fields (e.g., operatorCrds.install, validation.enabled, dbMigrate.enabled).
🧹 Nitpick comments (1)
values/caas-ci.yaml (1)
8-8: 💤 Low valueVerify image tag format:
sha-*tags are unconventional.The image tags use a
sha-<shortcommit>format (e.g.,sha-fe83a9f,sha-f2cd619,sha-ffe8959) which appears to reference git commit SHAs. While this may work if your registry supports these as tags, it's unconventional compared to:
- Semantic version tags (e.g.,
v1.2.3,0.1.0)- Full Docker image digests (e.g.,
@sha256:abc123...)Confirm this tag format is intentionally supported by your CI/CD pipeline and container registry.
Also applies to: 26-26, 65-65, 79-79
🤖 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 `@values/caas-ci.yaml` at line 8, The image tag values like "tag: sha-fe83a9f" (and the other occurrences at the same file) use a short git-sha prefixed with "sha-" which is unconventional; update the CI values/caas-ci.yaml entries that set tag to either a supported semantic version (e.g., "v1.2.3") or a full image digest (e.g., "@sha256:..."), or if you intentionally use short git tags ensure the CI/CD pipeline and container registry accept them and add validation to produce the exact tag format; locate the "tag: sha-..." lines and replace them with the agreed canonical format (or add pipeline/registry compatibility checks) for each occurrence.
🤖 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 `@charts/osac/templates/NOTES.txt`:
- Line 31: Replace the hardcoded relative chart path "charts/osac/" in NOTES.txt
with a clear placeholder (e.g. "<chart-path>" or "<path-to-chart>") and update
the example command and text to tell users to substitute their chart location or
run from the repo root; locate the string "charts/osac/" in templates/NOTES.txt
and change it to the placeholder and accompanying note so the helm upgrade
example works from any working directory.
---
Duplicate comments:
In `@charts/osac/values.schema.json`:
- Around line 72-76: The schema field insecureSkipVerify is incorrectly declared
as type "string" with default "true"; change its schema entry in
values.schema.json to type "boolean" and default true (unquoted) and update the
corresponding values files that set insecureSkipVerify to use the boolean true
instead of the string "true" so validation aligns with other boolean fields
(e.g., operatorCrds.install, validation.enabled, dbMigrate.enabled).
---
Nitpick comments:
In `@values/caas-ci.yaml`:
- Line 8: The image tag values like "tag: sha-fe83a9f" (and the other
occurrences at the same file) use a short git-sha prefixed with "sha-" which is
unconventional; update the CI values/caas-ci.yaml entries that set tag to either
a supported semantic version (e.g., "v1.2.3") or a full image digest (e.g.,
"@sha256:..."), or if you intentionally use short git tags ensure the CI/CD
pipeline and container registry accept them and add validation to produce the
exact tag format; locate the "tag: sha-..." lines and replace them with the
agreed canonical format (or add pipeline/registry compatibility checks) for each
occurrence.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f08fad88-a1dd-4185-b34e-09bf80708beb
📒 Files selected for processing (14)
.github/workflows/helm-integration.yaml.github/workflows/helm-lint.yaml.pre-commit-config.yamlbase/osac-aapbase/osac-fulfillment-servicebase/osac-operatorcharts/osac/Chart.yamlcharts/osac/templates/NOTES.txtcharts/osac/values.schema.jsoncharts/osac/values.yamlscripts/setup.shvalues/caas-ci.yamlvalues/development.yamlvalues/vmaas-ci.yaml
✅ Files skipped from review due to trivial changes (4)
- base/osac-aap
- base/osac-fulfillment-service
- .pre-commit-config.yaml
- charts/osac/values.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/setup.sh
- .github/workflows/helm-integration.yaml
- .github/workflows/helm-lint.yaml
b515490 to
f2b037a
Compare
ba49f4e to
8f220e7
Compare
eliorerz
changed the title
| {{- end }} | ||
| {{- if .Values.aap.bootstrap.enabled }} | ||
| - AAP bootstrap job (post-install hook) | ||
| {{- end }} |
There was a problem hiding this comment.
Regarding the installed components, I wanted to mention that initially we intended to make the system scalable by being able to have multiple "hubs" connected to the fulfillment service. But in this architecture it looks like we assume that there is only one cluster that runs the fulfillment service and all the other components. That is probably OK for now, but conceptually wrong. The fullfillment service, the database and Keycloak need a cluster (or a VM, or a set of machines) to run, and the other components (AAP the operator, etc) are only needed in the "hub" clusters. Not saying you should tackle that now, but keep it in the back of your mind.
8f220e7 to
a64aac7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@overlays/vmaas-ci/kustomization.yaml`:
- Around line 45-47: The overlay contains invalid artifact references: the
AAP_EE_IMAGE value (ghcr.io/osac-project/osac-aap:sha-acd61c8) and
AAP_PROJECT_GIT_BRANCH commit (acd61c850f78924dfadcea074fa43ba96fa6be6f)
referenced in the vmaas-ci kustomization (variables AAP_EE_IMAGE,
AAP_PROJECT_GIT_URI, AAP_PROJECT_GIT_BRANCH) do not exist; fix by either
updating these variables to point to an existing image tag and a valid commit
SHA/branch in the osac-aap repo, or ensure PR `#289` is merged and the image is
built/pushed so the exact tag/commit exist, then update all three CI overlays
(caas-ci, osac-integration, vmaas-ci) consistently to the corrected values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e2cb6c7-f5f0-4349-b35d-579ca09fbcdf
📒 Files selected for processing (23)
.github/workflows/helm-integration.yaml.github/workflows/helm-lint.yaml.pre-commit-config.yamlMakefileREADME.mdbase/kustomization.yamlbase/osac-aapbase/osac-fulfillment-servicebase/osac-operatorcharts/osac/Chart.yamlcharts/osac/templates/NOTES.txtcharts/osac/templates/_helpers.tplcharts/osac/templates/hooks/db-migrate.yamlcharts/osac/templates/hooks/pre-install-validate.yamlcharts/osac/values.schema.jsoncharts/osac/values.yamloverlays/caas-ci/kustomization.yamloverlays/osac-integration/kustomization.yamloverlays/vmaas-ci/kustomization.yamlscripts/setup.shvalues/caas-ci.yamlvalues/development.yamlvalues/vmaas-ci.yaml
✅ Files skipped from review due to trivial changes (5)
- base/kustomization.yaml
- .pre-commit-config.yaml
- base/osac-fulfillment-service
- charts/osac/templates/NOTES.txt
- README.md
🚧 Files skipped from review as they are similar to previous changes (15)
- base/osac-operator
- base/osac-aap
- charts/osac/templates/hooks/db-migrate.yaml
- values/development.yaml
- .github/workflows/helm-lint.yaml
- Makefile
- charts/osac/Chart.yaml
- .github/workflows/helm-integration.yaml
- values/vmaas-ci.yaml
- charts/osac/templates/hooks/pre-install-validate.yaml
- values/caas-ci.yaml
- charts/osac/values.schema.json
- scripts/setup.sh
- charts/osac/values.yaml
- charts/osac/templates/_helpers.tpl
eliorerz
changed the title
|
@eliorerz: This pull request references OSAC-22 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 epic 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. |
a64aac7 to
0058afe
Compare
4a428f4 to
eea62d8
Compare
|
/retest |
Add the umbrella chart at charts/osac/ that composes all OSAC component charts (operator CRDs, operator, fulfillment service, AAP) via file:// dependencies through git submodules. Includes: - Chart.yaml with dependencies on all component charts - Unified values.yaml with sub-chart value forwarding - values.schema.json for OpenShift Software Catalog form UI - Pre-install validation hook (checks cert-manager CRDs and StorageClass) - Pre-upgrade db-migrate hook (disabled until migrate cmd is released) - NOTES.txt with management plane vs hub component separation - Template helpers
- Create values/development.yaml, values/caas-ci.yaml, values/vmaas-ci.yaml replacing Kustomize overlays with Helm values - Include auth.controllerCredentials and postgres client cert mTLS configuration discovered during real-cluster testing - Disable dbMigrate hook (migrate subcommand not yet released) - Update scripts/setup.sh to support dual DEPLOY_MODE (helm/kustomize) - Add Makefile with sync-charts, helm-lint, helm-deploy targets
- helm-lint.yaml: lint and template validation on PRs - helm-integration.yaml: periodic kind-cluster deployment test - Update .pre-commit-config.yaml with YAML linting
eea62d8 to
800bc9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
charts/osac/templates/hooks/pre-install-validate.yaml (1)
69-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake cluster-scoped RBAC names namespace-unique.
Line 69/87/97 use a cluster-global name derived only from release fullname. Two installs with same release name in different namespaces can conflict and fail hook creation.
Suggested patch
+{{- $validateClusterName := printf "%s-%s-validate" (include "osac.fullname" .) .Release.Namespace | trunc 63 | trimSuffix "-" }} apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: - name: {{ include "osac.fullname" . }}-validate + name: {{ $validateClusterName }} @@ kind: ClusterRoleBinding metadata: - name: {{ include "osac.fullname" . }}-validate + name: {{ $validateClusterName }} @@ roleRef: apiGroup: rbac.authorization.k8s.io kind: ClusterRole - name: {{ include "osac.fullname" . }}-validate + name: {{ $validateClusterName }}As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 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/templates/hooks/pre-install-validate.yaml` around lines 69 - 97, The cluster-scoped RBAC resources use a global name from {{ include "osac.fullname" . }}-validate which can collide across namespaces; make these names namespace-unique by appending the release namespace (e.g. -{{ .Release.Namespace }}). Update the ClusterRole resource name, the ClusterRoleBinding.metadata.name and the ClusterRoleBinding.roleRef.name that currently use {{ include "osac.fullname" . }}-validate to instead use a namespaced form like {{ include "osac.fullname" . }}-validate-{{ .Release.Namespace }} so two installs in different namespaces won't conflict.charts/osac/templates/hooks/db-migrate.yaml (1)
27-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce exactly one DB migration input at render time.
Line 27 and Line 31 currently allow both
dbUrlanddbUrlFile(or neither), which can break upgrades at runtime. Add Helmfailguards so exactly one is set whendbMigrate.enabled=true.Suggested patch
{{- if .Values.dbMigrate.enabled }} +{{- if and .Values.dbMigrate.dbUrl .Values.dbMigrate.dbUrlFile }} +{{- fail "Set only one of dbMigrate.dbUrl or dbMigrate.dbUrlFile" }} +{{- end }} +{{- if and (not .Values.dbMigrate.dbUrl) (not .Values.dbMigrate.dbUrlFile) }} +{{- fail "When dbMigrate.enabled=true, set dbMigrate.dbUrl or dbMigrate.dbUrlFile" }} +{{- end }} apiVersion: batch/v1 kind: JobAs per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 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/templates/hooks/db-migrate.yaml` around lines 27 - 34, When db migrations are enabled, enforce exactly one of .Values.dbMigrate.dbUrl or .Values.dbMigrate.dbUrlFile is provided by adding Helm template guards that call fail() if the condition is violated; update the template around the dbUrl/dbUrlFile blocks (referencing .Values.dbMigrate.enabled, .Values.dbMigrate.dbUrl, .Values.dbMigrate.dbUrlFile) to check: if .Values.dbMigrate.enabled is true then compute whether both are set or neither are set and call fail("...") with a clear message when that occurs, otherwise render the existing --db-url or --db-url-file lines as before.
🤖 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 19-20: The CI step named "Update submodules to latest main"
currently uses git submodule update --remote which moves submodule heads and
breaks determinism; change it to update submodules to the commits pinned by the
superproject (e.g., use git submodule update --init --recursive so submodules
are checked out at the SHAs recorded in the PR rather than tracking remote
branches) so the integration job tests the PR's pinned submodule SHAs reliably.
- Around line 14-30: Replace the three action version tags with pinned commit
SHAs: change uses: actions/checkout@v6 to uses:
actions/checkout@<full-commit-sha>, uses: azure/setup-helm@v4 to uses:
azure/setup-helm@<full-commit-sha>, and uses: helm/kind-action@v1 to uses:
helm/kind-action@<full-commit-sha>; additionally, add persist-credentials: false
to the actions/checkout step (under the with block) to disable persisted
credentials. Ensure you fetch the corresponding full commit SHAs from each
action's official repo and update the "uses" entries accordingly while leaving
other inputs (like version and cluster_name) unchanged.
In @.github/workflows/helm-lint.yaml:
- Around line 15-23: Replace the mutable action tags with exact commit SHAs and
disable credential persistence for checkout: update the actions/checkout
reference used by the job (currently "uses: actions/checkout@v6") to the full
commit SHA for that action and add "persist-credentials: false" under its with
block; likewise replace "uses: azure/setup-helm@v4" with the full commit SHA for
the azure/setup-helm action so both actions are pinned to immutable SHAs.
In `@scripts/setup.sh`:
- Around line 305-309: The script currently only calls wait_for_resource for
job/aap-bootstrap when DEPLOY_MODE == "kustomize", causing race conditions in
Helm mode; modify scripts/setup.sh so that wait_for_resource job/aap-bootstrap
condition=complete 2400 "${INSTALLER_NAMESPACE}" is also executed before running
post-install steps (e.g., before invoking prepare-aap.sh) when using
Helm/DEPLOY_MODE != "kustomize" (or unconditionally after install), ensuring the
bootstrap job is complete despite helm --wait not waiting for hook jobs; locate
the DEPLOY_MODE check and the post-install invocation of prepare-aap.sh and add
the wait_for_resource call in the Helm path (or move the existing call to run
for both paths).
---
Duplicate comments:
In `@charts/osac/templates/hooks/db-migrate.yaml`:
- Around line 27-34: When db migrations are enabled, enforce exactly one of
.Values.dbMigrate.dbUrl or .Values.dbMigrate.dbUrlFile is provided by adding
Helm template guards that call fail() if the condition is violated; update the
template around the dbUrl/dbUrlFile blocks (referencing
.Values.dbMigrate.enabled, .Values.dbMigrate.dbUrl, .Values.dbMigrate.dbUrlFile)
to check: if .Values.dbMigrate.enabled is true then compute whether both are set
or neither are set and call fail("...") with a clear message when that occurs,
otherwise render the existing --db-url or --db-url-file lines as before.
In `@charts/osac/templates/hooks/pre-install-validate.yaml`:
- Around line 69-97: The cluster-scoped RBAC resources use a global name from {{
include "osac.fullname" . }}-validate which can collide across namespaces; make
these names namespace-unique by appending the release namespace (e.g. -{{
.Release.Namespace }}). Update the ClusterRole resource name, the
ClusterRoleBinding.metadata.name and the ClusterRoleBinding.roleRef.name that
currently use {{ include "osac.fullname" . }}-validate to instead use a
namespaced form like {{ include "osac.fullname" . }}-validate-{{
.Release.Namespace }} so two installs in different namespaces won't conflict.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: aed3e97b-dec6-44e3-a04e-b614dcbc74bd
📒 Files selected for processing (17)
.github/workflows/helm-integration.yaml.github/workflows/helm-lint.yaml.pre-commit-config.yamlMakefileREADME.mdcharts/osac/Chart.yamlcharts/osac/templates/NOTES.txtcharts/osac/templates/_helpers.tplcharts/osac/templates/hooks/db-migrate.yamlcharts/osac/templates/hooks/pre-install-validate.yamlcharts/osac/values.schema.jsoncharts/osac/values.yamldocs/helm-deployment-guide.mdscripts/setup.shvalues/caas-ci.yamlvalues/development.yamlvalues/vmaas-ci.yaml
✅ Files skipped from review due to trivial changes (4)
- charts/osac/templates/NOTES.txt
- .pre-commit-config.yaml
- charts/osac/values.yaml
- values/caas-ci.yaml
|
@eliorerz: The following tests 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. |
800bc9a to
1b60ca3
Compare
- Add docs/helm-deployment-guide.md with comprehensive step-by-step instructions for deploying OSAC on a clean OpenShift cluster - Cover prerequisites, secrets (CA trust, postgres mTLS, credentials), Helm deploy, and post-install configuration - Document known issues: OPA policy mismatch (OSAC-903), missing postgres client certs (OSAC-904), missing DB secret (OSAC-905) - Update README.md with Helm installation quick start - Bump submodules to latest main - Update Kustomize overlay image tags
1b60ca3 to
35c1c21
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eliorerz, jhernand 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 |
|
/override ci/prow/e2e-vmaas |
|
@eliorerz: Overrode contexts on behalf of eliorerz: ci/prow/e2e-vmaas 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 kubernetes-sigs/prow repository. |
4 participants
Summary
Replaces the Kustomize-based installer with a Helm umbrella chart pattern for deploying OSAC. Part of OSAC-22.
What this PR adds:
charts/osac/) — composes operator CRDs, operator, fulfillment service, and AAP charts viafile://dependencies through git submodulesvalues/) — development, CaaS CI, and VMaaS CI profiles replacing Kustomize overlaysDEPLOY_MODEsupport (helm/kustomize) inscripts/setup.shsync-charts,helm-lint,helm-deploy)helm-lint.yaml(PR validation) andhelm-integration.yaml(periodic kind-cluster test)docs/helm-deployment-guide.md) — step-by-step instructions for deploying on a clean OpenShift cluster, including prerequisites, secrets, and post-install configurationArchitecture note: The chart currently co-deploys the management plane (fulfillment service) and hub components (operator, AAP) to the same cluster. In a future multi-hub architecture, these would be deployed separately.
Commits
Test plan
helm dependency build charts/osac/ && helm lint charts/osac/passeshelm template osac charts/osac/ --values values/development.yamlrenders without errors