Add crd only chart, that installs CRDs unconditionally#276
Open
Add crd only chart, that installs CRDs unconditionally#276
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughAdds a Helm chart scaffold for CRDs and a new GitHub Actions workflow that lints, packages, and publishes the CRD chart as an OCI artifact to GHCR when CRD files change or on manual dispatch. Changes
Sequence Diagram(s)sequenceDiagram
participant Repo as Repository
participant Actions as GitHub Actions Runner
participant Helm as Helm CLI
participant GHCR as ghcr.io (OCI Registry)
Repo->>Actions: push to main (charts/.../crds/**) / manual dispatch
Actions->>Actions: checkout repo
Actions->>Helm: install Helm
Actions->>Actions: copy `charts/.../crds/*.yaml` -> `charts/...-crd/templates/`
Actions->>Helm: helm lint charts/...-crd
Actions->>Helm: helm package with version 1.0.${{run_number}} -> ./chart/*.tgz
Actions->>GHCR: docker/login using GITHUB_TOKEN
Actions->>GHCR: helm push ./chart/*.tgz to oci://ghcr.io/${{ github.repository_owner }}/charts
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
notandy
force-pushed
the
add-seperate-crd-charts
branch
from
a035e67 to
528f623
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/helm-crd-oci-package-ghcr.yaml (1)
27-29: Consider documenting CRD installation options for users.Both the main operator chart (via
crds/directory) and this CRD-only chart install the same CRDs. Users installing both charts would get duplicate CRD applications. This is generally safe (idempotent), but consider adding documentation clarifying:
- Use the CRD chart for pre-installing CRDs before the operator (e.g., for GitOps flows where CRDs must exist first)
- The main operator chart also bundles CRDs
This is architectural guidance rather than a code issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helm-crd-oci-package-ghcr.yaml around lines 27 - 29, Add documentation clarifying CRD installation options to avoid user confusion between the operator chart and the CRD-only chart: update the README (or docs) to explain that the CRD chart (charts/openstack-hypervisor-operator-crd/templates/) is intended for pre-installing CRDs (useful for GitOps or when CRDs must exist before the operator), while the main operator chart also bundles the same CRDs in charts/openstack-hypervisor-operator/crds/ and will apply them during install; reference the workflow step "Sync CRDs from main chart" in .github/workflows/helm-crd-oci-package-ghcr.yaml as the reason CRDs are duplicated and recommend guidance for users on which chart to use in common scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/helm-crd-oci-package-ghcr.yaml:
- Around line 27-29: Add documentation clarifying CRD installation options to
avoid user confusion between the operator chart and the CRD-only chart: update
the README (or docs) to explain that the CRD chart
(charts/openstack-hypervisor-operator-crd/templates/) is intended for
pre-installing CRDs (useful for GitOps or when CRDs must exist before the
operator), while the main operator chart also bundles the same CRDs in
charts/openstack-hypervisor-operator/crds/ and will apply them during install;
reference the workflow step "Sync CRDs from main chart" in
.github/workflows/helm-crd-oci-package-ghcr.yaml as the reason CRDs are
duplicated and recommend guidance for users on which chart to use in common
scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0df7f3c-04f4-4d7e-9c7e-592bf4a0807f
📒 Files selected for processing (5)
.github/workflows/helm-crd-oci-package-ghcr.yamlcharts/openstack-hypervisor-operator-crd/.helmignorecharts/openstack-hypervisor-operator-crd/Chart.yamlcharts/openstack-hypervisor-operator-crd/templates/kvm.cloud.sap_evictions.yamlcharts/openstack-hypervisor-operator-crd/templates/kvm.cloud.sap_hypervisors.yaml
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
.github/workflows/helm-crd-oci-package-ghcr.yaml (4)
20-24: Consider removing unnecessary checkout options.The workflow fetches full git history (
fetch-depth: 0) and tags (fetch-tags: true), but versioning usesgithub.run_numberrather than git tags or commit history. Removing these options would speed up the checkout step.⚡ Proposed optimization
- name: Check out code uses: actions/checkout@v6 - with: - fetch-depth: 0 - fetch-tags: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helm-crd-oci-package-ghcr.yaml around lines 20 - 24, The checkout step named "Check out code" currently sets fetch-depth: 0 and fetch-tags: true which fetches full history and tags unnecessarily; remove those options (or set fetch-depth: 1) from the actions/checkout@v6 invocation so the workflow only fetches what’s needed for using github.run_number, improving performance and checkout speed.
25-26: Consider pinning the Helm version.While using the latest Helm version generally works, pinning to a specific version ensures consistent behavior across workflow runs and prevents potential issues from breaking changes in future Helm releases.
🔒 Example of version pinning
- name: Install Helm uses: azure/setup-helm@v4 + with: + version: 'v3.16.3' # or latest stable version🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helm-crd-oci-package-ghcr.yaml around lines 25 - 26, The workflow currently uses the reusable action reference azure/setup-helm@v4 in the "Install Helm" step without pinning a specific Helm release; update that step (the "Install Helm" step that uses azure/setup-helm@v4) to pin a concrete Helm version—either by switching the action reference to an immutable tag/commit or by passing a fixed version input (e.g., version: "3.x.y") to ensure reproducible runs and avoid breaking changes from future Helm releases.
44-45: Consider adding validation before pushing.The
helm pushcommand uses a wildcard (./chart/*.tgz) without verifying that the package was actually created in the previous step. While unlikely to fail in practice, adding validation improves reliability.🔍 Proposed improvement with validation
- name: Push Helm Chart to ghcr.io - run: helm push ./chart/*.tgz oci://ghcr.io/${{ github.repository_owner }}/charts + run: | + set -e + chart_file=$(ls -1 ./chart/*.tgz 2>/dev/null | head -1) + if [ -z "$chart_file" ]; then + echo "Error: No packaged chart found" + exit 1 + fi + echo "Pushing chart: $chart_file" + helm push "$chart_file" oci://ghcr.io/${{ github.repository_owner }}/charts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helm-crd-oci-package-ghcr.yaml around lines 44 - 45, The helm push step ("Push Helm Chart to ghcr.io") currently runs helm push ./chart/*.tgz oci://ghcr.io/${{ github.repository_owner }}/charts without checking that any .tgz was produced; add a pre-check step that verifies one or more ./chart/*.tgz files exist (and optionally are non-empty) and fail early with a clear error message if not found, then only run the helm push command when the validation succeeds so the job doesn’t attempt to push a missing artifact.
18-18: Consider usingubuntu-latestfor this Helm-only workflow to reduce resource costs.This workflow only performs lightweight Helm operations (lint, package, and push). While
large_runner_16core_64gbis used consistently across the organization, it's overkill for these tasks. A standard GitHub-hosted runner likeubuntu-latestwould be sufficient and more cost-effective.If the large runner is needed for specific reasons (network speed, organizational policy, etc.), consider documenting why in a workflow comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/helm-crd-oci-package-ghcr.yaml at line 18, The workflow currently uses an oversized runner via the runs-on key; change runs-on: large_runner_16core_64gb to runs-on: ubuntu-latest to use the standard GitHub-hosted runner for this Helm-only workflow (lint/package/push), and if the large runner is truly required, keep the existing value but add a comment near the runs-on setting explaining the specific justification (e.g., network or policy) so the choice is documented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/helm-crd-oci-package-ghcr.yaml:
- Around line 6-10: The workflow's push path filter only watches
'charts/openstack-hypervisor-operator/crds/**' so updates to the CRD-only chart
metadata (e.g., charts/openstack-hypervisor-operator-crd/Chart.yaml) won't
trigger the job; update the paths array to also include the CRD-chart metadata
(for example add charts/openstack-hypervisor-operator-crd/** or at minimum
charts/openstack-hypervisor-operator-crd/Chart.yaml) so metadata-only changes to
the CRD chart will fire this workflow.
- Around line 27-29: The "Sync CRDs from main chart" step currently runs a bare
cp with a wildcard and doesn't verify success; update that step to first ensure
source glob matches and that files were actually copied by adding a post-copy
validation that checks the destination directory
(charts/openstack-hypervisor-operator-crd/templates) contains at least one .yaml
file and exits non-zero with an error message if not; reference the existing cp
invocation in that step and add a conditional check (e.g., test for files with
ls or a shell glob check) right after it so the workflow fails fast when no CRDs
were copied.
- Around line 32-37: The workflow allows manual runs via workflow_dispatch which
will still increment VERSION="1.0.${{ github.run_number }}" without CRD changes;
either remove the workflow_dispatch trigger, or make it require deliberate input
(e.g., a confirmation or changelog input) before proceeding, or change the
VERSION derivation used in the helm package step (the VERSION variable and the
helm package charts/openstack-hypervisor-operator-crd --version "$VERSION"
invocation) to be based on a git-derived identifier (commit SHA or tag) so
manual dispatches do not silently advance the CRD package version; implement one
of these fixes and ensure the workflow gate checks the new input or uses git
metadata before running the helm package step.
---
Nitpick comments:
In @.github/workflows/helm-crd-oci-package-ghcr.yaml:
- Around line 20-24: The checkout step named "Check out code" currently sets
fetch-depth: 0 and fetch-tags: true which fetches full history and tags
unnecessarily; remove those options (or set fetch-depth: 1) from the
actions/checkout@v6 invocation so the workflow only fetches what’s needed for
using github.run_number, improving performance and checkout speed.
- Around line 25-26: The workflow currently uses the reusable action reference
azure/setup-helm@v4 in the "Install Helm" step without pinning a specific Helm
release; update that step (the "Install Helm" step that uses
azure/setup-helm@v4) to pin a concrete Helm version—either by switching the
action reference to an immutable tag/commit or by passing a fixed version input
(e.g., version: "3.x.y") to ensure reproducible runs and avoid breaking changes
from future Helm releases.
- Around line 44-45: The helm push step ("Push Helm Chart to ghcr.io") currently
runs helm push ./chart/*.tgz oci://ghcr.io/${{ github.repository_owner }}/charts
without checking that any .tgz was produced; add a pre-check step that verifies
one or more ./chart/*.tgz files exist (and optionally are non-empty) and fail
early with a clear error message if not found, then only run the helm push
command when the validation succeeds so the job doesn’t attempt to push a
missing artifact.
- Line 18: The workflow currently uses an oversized runner via the runs-on key;
change runs-on: large_runner_16core_64gb to runs-on: ubuntu-latest to use the
standard GitHub-hosted runner for this Helm-only workflow (lint/package/push),
and if the large runner is truly required, keep the existing value but add a
comment near the runs-on setting explaining the specific justification (e.g.,
network or policy) so the choice is documented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 899b4d4e-2847-43c3-86f2-471ba489fb79
📒 Files selected for processing (4)
.github/workflows/helm-crd-oci-package-ghcr.yamlcharts/openstack-hypervisor-operator-crd/.helmignorecharts/openstack-hypervisor-operator-crd/Chart.yamlcharts/openstack-hypervisor-operator-crd/templates/.empty
✅ Files skipped from review due to trivial changes (2)
- charts/openstack-hypervisor-operator-crd/.helmignore
- charts/openstack-hypervisor-operator-crd/Chart.yaml
Comment on lines
+6
to
+10
| push: | ||
| branches: | ||
| - main | ||
| paths: | ||
| - 'charts/openstack-hypervisor-operator/crds/**' |
There was a problem hiding this comment.
Consider adding the CRD chart metadata to the path filter.
The current path filter only triggers when CRDs in the main chart change. Updates to the CRD-only chart metadata (e.g., charts/openstack-hypervisor-operator-crd/Chart.yaml) won't trigger this workflow, preventing you from publishing metadata-only updates.
📝 Proposed fix to include CRD chart metadata
push:
branches:
- main
paths:
- 'charts/openstack-hypervisor-operator/crds/**'
+ - 'charts/openstack-hypervisor-operator-crd/**'
workflow_dispatch: {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/helm-crd-oci-package-ghcr.yaml around lines 6 - 10, The
workflow's push path filter only watches
'charts/openstack-hypervisor-operator/crds/**' so updates to the CRD-only chart
metadata (e.g., charts/openstack-hypervisor-operator-crd/Chart.yaml) won't
trigger the job; update the paths array to also include the CRD-chart metadata
(for example add charts/openstack-hypervisor-operator-crd/** or at minimum
charts/openstack-hypervisor-operator-crd/Chart.yaml) so metadata-only changes to
the CRD chart will fire this workflow.
Comment on lines
+27
to
+29
| - name: Sync CRDs from main chart | ||
| run: | | ||
| cp charts/openstack-hypervisor-operator/crds/*.yaml charts/openstack-hypervisor-operator-crd/templates/ |
There was a problem hiding this comment.
Add validation that CRDs were copied successfully.
The cp command uses a wildcard but doesn't verify that any files were actually copied. If the source directory is empty or the copy fails, the workflow would proceed to package an incomplete chart.
✅ Proposed fix with validation
- name: Sync CRDs from main chart
run: |
+ set -e
cp charts/openstack-hypervisor-operator/crds/*.yaml charts/openstack-hypervisor-operator-crd/templates/
+ # Verify CRDs were copied
+ crd_count=$(ls -1 charts/openstack-hypervisor-operator-crd/templates/*.yaml 2>/dev/null | wc -l)
+ if [ "$crd_count" -eq 0 ]; then
+ echo "Error: No CRD files were copied"
+ exit 1
+ fi
+ echo "Successfully copied $crd_count CRD file(s)"📝 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.
Suggested change
| - name: Sync CRDs from main chart | |
| run: | | |
| cp charts/openstack-hypervisor-operator/crds/*.yaml charts/openstack-hypervisor-operator-crd/templates/ | |
| - name: Sync CRDs from main chart | |
| run: | | |
| set -e | |
| cp charts/openstack-hypervisor-operator/crds/*.yaml charts/openstack-hypervisor-operator-crd/templates/ | |
| # Verify CRDs were copied | |
| crd_count=$(ls -1 charts/openstack-hypervisor-operator-crd/templates/*.yaml 2>/dev/null | wc -l) | |
| if [ "$crd_count" -eq 0 ]; then | |
| echo "Error: No CRD files were copied" | |
| exit 1 | |
| fi | |
| echo "Successfully copied $crd_count CRD file(s)" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/helm-crd-oci-package-ghcr.yaml around lines 27 - 29, The
"Sync CRDs from main chart" step currently runs a bare cp with a wildcard and
doesn't verify success; update that step to first ensure source glob matches and
that files were actually copied by adding a post-copy validation that checks the
destination directory (charts/openstack-hypervisor-operator-crd/templates)
contains at least one .yaml file and exits non-zero with an error message if
not; reference the existing cp invocation in that step and add a conditional
check (e.g., test for files with ls or a shell glob check) right after it so the
workflow fails fast when no CRDs were copied.
Comment on lines
+32
to
+37
| - name: Package Helm Chart | ||
| run: | | ||
| # Use run number to auto-increment version on each CRD change | ||
| VERSION="1.0.${{ github.run_number }}" | ||
| echo "Running helm package with version $VERSION" | ||
| helm package charts/openstack-hypervisor-operator-crd --destination ./chart --version "$VERSION" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat .github/workflows/helm-crd-oci-package-ghcr.yamlRepository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 1673
🏁 Script executed:
# Check the overall structure and triggers of the workflow
head -50 .github/workflows/helm-crd-oci-package-ghcr.yamlRepository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 1673
🏁 Script executed:
# Look for any CRD change detection logic in the workflow
grep -n "crd\|CRD\|paths\|on:" .github/workflows/helm-crd-oci-package-ghcr.yamlRepository: cobaltcore-dev/openstack-hypervisor-operator
Length of output: 733
Address the risk of versioning without actual code changes via workflow_dispatch.
The workflow correctly triggers on CRD file changes via the paths filter for push events. However, workflow_dispatch: {} allows manual triggering that increments the version without any code changes. This defeats the stated goal of "auto-versioned whenever a CRD changes."
Consider either:
- Removing
workflow_dispatch: {}to ensure versions only increment on actual CRD changes - Adding manual inputs to
workflow_dispatch(e.g., a confirmation or changelog) to enforce intentionality - Using git-based versioning (commit SHA, tags) for better traceability to the actual CRD state
The current approach of github.run_number is adequate for sequencing within this workflow's push context, but the manual dispatch loophole should be closed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/helm-crd-oci-package-ghcr.yaml around lines 32 - 37, The
workflow allows manual runs via workflow_dispatch which will still increment
VERSION="1.0.${{ github.run_number }}" without CRD changes; either remove the
workflow_dispatch trigger, or make it require deliberate input (e.g., a
confirmation or changelog input) before proceeding, or change the VERSION
derivation used in the helm package step (the VERSION variable and the helm
package charts/openstack-hypervisor-operator-crd --version "$VERSION"
invocation) to be based on a git-derived identifier (commit SHA or tag) so
manual dispatches do not silently advance the CRD package version; implement one
of these fixes and ensure the workflow gate checks the new input or uses git
metadata before running the helm package step.
not happy with that, but it seems that's the preferable way currently. Chart is auto-version on every CRD changed and pushed to ghcr.
notandy
force-pushed
the
add-seperate-crd-charts
branch
from
528f623 to
bea07ce
Compare
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
not happy with that, but it seems that's the preferable way currently.
Chart is auto-version on every CRD changed and pushed to ghcr.
Summary by CodeRabbit
New Features
Chores