This repository was archived by the owner on Sep 9, 2026. It is now read-only.
OSAC-502: Create osac-operator CRDs Helm chart - #226
Merged
openshift-merge-bot[bot] merged 2 commits intoMay 13, 2026
Merged
openshift-merge-bot[bot] merged 2 commits into
openshift-merge-bot[bot] merged 2 commits into
Conversation
Add a Helm chart at charts/operator-crds/ packaging all 8 CRDs for Helm-based deployment. CRDs are placed in templates/ (not crds/) so they update on helm upgrade, with helm.sh/resource-policy: keep to prevent deletion on uninstall. Also adds helm-crds Makefile target to sync auto-generated CRDs from config/crd/bases/ into the chart, plus CI workflows for helm lint and chart publishing to OCI registry.
|
@eliorerz: This pull request references OSAC-502 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. |
📝 WalkthroughWalkthroughThis PR adds Helm charts for the OSAC operator and CRDs, helper templates, a Makefile target to sync generated CRDs into chart templates, operator deployment and RBAC templates, chart values, and GitHub Actions workflows for linting and publishing charts. ChangesHelm Charts & CRD Deployment Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (push tag)
participant GHA as GitHub Actions
participant Helm as Helm
participant GHCR as GHCR
Dev->>GHA: push tag vX
GHA->>Helm: check out + setup Helm v3.17.0
GHA->>Helm: extract version, package charts/operator-crds
GHA->>GHCR: push CRDs OCI artifact
GHA->>Helm: update operator values (image tag vX), package charts/operator
GHA->>GHCR: push operator OCI artifact
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
.github/workflows/helm-lint.yaml (1)
5-6: ⚡ Quick winExpand PR path filters to include CRD generation inputs.
Right now linting only runs when files undercharts/**change. Ifconfig/crd/bases/**orMakefilechanges, chart sync/render regressions can slip through without this workflow running.Suggested diff
on: pull_request: paths: - 'charts/**' + - 'config/crd/bases/**' + - 'Makefile' + - '.github/workflows/helm-lint.yaml'🤖 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-lint.yaml around lines 5 - 6, The workflow's path filter only triggers on 'paths: - 'charts/**'', so updates to CRD inputs or the Makefile won't run the helm-lint job; update the paths block to include the CRD sources and Makefile (add globs like 'config/crd/bases/**' and 'Makefile') so changes to CRD generation inputs trigger the workflow; ensure you keep the existing 'charts/**' entry and add the new entries to the same paths list.
🤖 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/operator-crds/templates/osac.openshift.io_clusterorders.yaml`:
- Around line 110-114: Fix the typo in the templateID field description: change
"identigier" to "identifier" in the CRD's templateID description (the block
under the templateID property in the ClusterOrders CRD template). Ensure the
sentence reads "TemplateID is the unique identifier of the cluster template to
use when creating this cluster" and keep the surrounding keys (templateID,
pattern, type) unchanged.
In `@charts/operator-crds/templates/osac.openshift.io_virtualnetworks.yaml`:
- Around line 207-220: The VirtualNetwork CRD's phase schema currently uses a
redundant allOf with two identical enum entries; update the schema for the phase
field (the "phase" property in osac.openshift.io_virtualnetworks.yaml) to use a
single plain enum with values Progressing, Ready, Failed, Deleting instead of
the allOf wrapper so it matches the other CRDs and avoids duplicate enum
definitions.
In `@charts/operator/templates/aggregate-roles.yaml`:
- Around line 1-48: The template is missing a ClusterRole for the
clusterorder-admin triad; add a new ClusterRole named {{ include
"osac-operator.fullname" . }}-clusterorder-admin mirroring the existing admin
roles (e.g., computeinstance-admin/tenant-admin) so clusterorders have
admin/editor/viewer parity: include rules for apiGroups: osac.openshift.io with
resources: clusterorders and clusterorders/status and grant the full admin verbs
used by other admin roles (create, delete, get, list, patch, update, watch — and
deletecollection if present in other admin definitions) to match the existing
admin role structure.
- Around line 185-199: This ClusterRoleBinding incorrectly grants the built-in
cluster-wide "admin" ClusterRole to the operator ServiceAccount; change it to a
namespace-scoped RoleBinding that references the operator's custom Role instead:
replace kind: ClusterRoleBinding with kind: RoleBinding, set roleRef.kind: Role,
and set roleRef.name to the custom admin role used in this chart (the same
logical name as the metadata name template, e.g. the {{ include
"osac-operator.fullname" . }}-admin role) so the binding is scoped to {{
.Release.Namespace }} and only grants namespace-scoped permissions to the
ServiceAccount named by {{ include "osac-operator.serviceAccountName" . }}.
- Around line 1-184: The ClusterRole resources (e.g., names including
-clusterorder-editor, -clusterorder-viewer, -computeinstance-admin,
-computeinstance-editor, -computeinstance-viewer, -tenant-admin, -tenant-editor,
-tenant-viewer) are missing the aggregation labels, so add the appropriate
rbac.authorization.k8s.io/aggregate-to-* labels to each ClusterRole: for admin
roles (e.g., *-admin) add rbac.authorization.k8s.io/aggregate-to-admin: "true";
for editor roles (e.g., *-editor, including clusterorder-editor,
computeinstance-editor, tenant-editor) add
rbac.authorization.k8s.io/aggregate-to-edit: "true"; for viewer roles (e.g.,
*-viewer) add rbac.authorization.k8s.io/aggregate-to-view: "true"; place these
labels under metadata.labels (merging with the existing include
"osac-operator.labels" entries) so Kubernetes will aggregate the rules into the
builtin admin/edit/view ClusterRoles.
In `@charts/operator/templates/deployment.yaml`:
- Around line 91-96: Add readOnlyRootFilesystem: true to the container's
securityContext to harden the operator pod; specifically, update the
securityContext block (where allowPrivilegeEscalation: false and
capabilities.drop: ["ALL"] are set) to include readOnlyRootFilesystem: true so
the container root filesystem is immutable at runtime.
In `@charts/operator/templates/leader-election-role.yaml`:
- Around line 14-20: Remove the unnecessary "delete" verb from the
leader-election Role's rules for the configmaps and leases resources: locate the
verbs lists for the configmaps rule (the rule referencing "configmaps" used by
leader-election) and the leases rule (the rule referencing "leases") in the
leader-election-role template and delete the "delete" entry so each verbs array
only contains get, list, watch, create, update (and patch if present); ensure no
other verbs are changed and the YAML remains valid.
In `@Makefile`:
- Around line 173-175: The replacement that injects the CRD annotation currently
only replaces the literal ' annotations:\n' in the variable content and does
nothing when an annotations block is absent; update the Makefile logic that
builds content (the branch that checks if 'helm.sh/resource-policy' not in
content and then does content.replace) to detect missing annotations and, when
absent, insert a metadata.annotations block under the CRD's metadata section
containing "helm.sh/resource-policy": keep (i.e., add a fallback path that
injects 'metadata:\n annotations:\n "helm.sh/resource-policy": keep\n' in
the CRD YAML), ensuring the existing replace path is kept for files that already
have annotations and only the new insert is used when the annotations key is
missing.
---
Nitpick comments:
In @.github/workflows/helm-lint.yaml:
- Around line 5-6: The workflow's path filter only triggers on 'paths: -
'charts/**'', so updates to CRD inputs or the Makefile won't run the helm-lint
job; update the paths block to include the CRD sources and Makefile (add globs
like 'config/crd/bases/**' and 'Makefile') so changes to CRD generation inputs
trigger the workflow; ensure you keep the existing 'charts/**' entry and add the
new entries to the same paths list.
🪄 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: 9bf6d9f6-a451-4562-907f-8452d5756429
📒 Files selected for processing (25)
.github/workflows/helm-lint.yaml.github/workflows/publish-charts.yamlMakefilecharts/operator-crds/Chart.yamlcharts/operator-crds/templates/_helpers.tplcharts/operator-crds/templates/osac.openshift.io_clusterorders.yamlcharts/operator-crds/templates/osac.openshift.io_computeinstances.yamlcharts/operator-crds/templates/osac.openshift.io_publicippools.yamlcharts/operator-crds/templates/osac.openshift.io_publicips.yamlcharts/operator-crds/templates/osac.openshift.io_securitygroups.yamlcharts/operator-crds/templates/osac.openshift.io_subnets.yamlcharts/operator-crds/templates/osac.openshift.io_tenants.yamlcharts/operator-crds/templates/osac.openshift.io_virtualnetworks.yamlcharts/operator-crds/values.yamlcharts/operator/Chart.yamlcharts/operator/templates/_helpers.tplcharts/operator/templates/aggregate-roles.yamlcharts/operator/templates/clusterrole.yamlcharts/operator/templates/clusterrolebinding.yamlcharts/operator/templates/deployment.yamlcharts/operator/templates/leader-election-role.yamlcharts/operator/templates/leader-election-rolebinding.yamlcharts/operator/templates/metrics-service.yamlcharts/operator/templates/serviceaccount.yamlcharts/operator/values.yaml
eliorerz
force-pushed
the
OSAC-502-create-operator-crds-helm-chart
branch
from
May 7, 2026 15:38
1d5fa1f to
25b4e34
Compare
Add a Helm chart at charts/operator/ translating Kustomize manifests from config/manager/, config/rbac/, and config/default/ into Helm templates. Includes Deployment, ClusterRole, ClusterRoleBinding, ServiceAccount, leader election, metrics service, and aggregate RBAC roles for ClusterOrder, ComputeInstance, and Tenant resources. All Kustomize namePrefix/namespace references are replaced with Helm template functions, and operator configuration is parameterized through values.yaml.
eliorerz
force-pushed
the
OSAC-502-create-operator-crds-helm-chart
branch
from
May 8, 2026 13:02
25b4e34 to
9cd1fca
Compare
eliorerz
changed the title
Contributor
Author
|
/retest |
|
@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. |
Contributor
Author
|
/retest |
Merged
7 tasks
jhernand
approved these changes
May 13, 2026
|
[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 |
12 tasks
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
OSAC is transitioning from Kustomize to Helm for installation. This PR adds the operator's Helm charts so they can be consumed by the umbrella installer chart.
Summary
charts/operator-crds/Helm chart packaging all 8 CRDs intemplates/withhelm.sh/resource-policy: keepannotationcharts/operator/Helm chart translating Kustomize manifests (Deployment, RBAC, ServiceAccount, leader election, metrics, aggregate roles) into parameterized Helm templateshelm-crdsMakefile target to sync auto-generated CRDs into the chartTest plan
helm lint charts/operator-crds/passeshelm lint charts/operator/passeshelm template test charts/operator-crds/renders all 8 CRDshelm template test charts/operator/renders all resourcesmake helm-crdssyncs CRDs from config/crd/bases/Summary by CodeRabbit
Release Notes