[ISSUE #1345]🔨Update Github Actions CI #1346
Merged
Conversation
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
WalkthroughThe changes in this pull request involve modifications to the GitHub Actions workflow file Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
|
|
🔊@mxsm 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
rocketmq-rust-bot
added
AI review first
SpaceXCN
approved these changes
Nov 26, 2024
rocketmq-rust-bot
added
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/update-labels.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/update-labels.yml
23-23: could not parse as YAML: yaml: line 23: mapping values are not allowed in this context
(syntax-check)
🪛 yamllint (1.35.1)
.github/workflows/update-labels.yml
[error] 23-23: syntax error: mapping values are not allowed here
(syntax)
🔇 Additional comments (1)
.github/workflows/update-labels.yml (1)
Line range hint 1-78: Verify security configuration and action versions
Please verify:
- The
BOT_TOKENhas minimal required permissions - The GitHub Actions versions are pinned to SHA for security
🧰 Tools
🪛 actionlint (1.7.4)
23-23: could not parse as YAML: yaml: line 23: mapping values are not allowed in this context
(syntax-check)
🪛 yamllint (1.35.1)
[error] 23-23: syntax error: mapping values are not allowed here
(syntax)
Comment on lines
+22
to
+24
| - name: Debug Workflow Context | ||
| run: echo "Payload: $(cat $GITHUB_EVENT_PATH)" | ||
|
|
There was a problem hiding this comment.
Fix YAML syntax and consider security implications
- The
runcommand needs proper YAML indentation - Exposing the full event payload in logs could leak sensitive information
Apply this fix for proper YAML syntax:
- name: Debug Workflow Context
- run: echo "Payload: $(cat $GITHUB_EVENT_PATH)"
+ run: |
+ # Filter sensitive information before logging
+ jq '. | del(.token) | del(.secrets)' $GITHUB_EVENT_PATHCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 actionlint (1.7.4)
23-23: could not parse as YAML: yaml: line 23: mapping values are not allowed in this context
(syntax-check)
🪛 yamllint (1.35.1)
[error] 23-23: syntax error: mapping values are not allowed here
(syntax)
Comment on lines
+33
to
+44
| const workflowRunId = context.payload.workflow_run.id; | ||
| const { data: workflowRun } = await github.rest.actions.getWorkflowRun({ | ||
| owner, | ||
| repo, | ||
| issue_number, | ||
| run_id: workflowRunId, | ||
| }); | ||
|
|
||
| // Filter the labels that exist on the PR and are in the labelsToRemove list | ||
| const existingLabelsToRemove = labels | ||
| .filter(label => labelsToRemove.includes(label.name)) | ||
| .map(label => label.name); | ||
|
|
||
| // Remove each label | ||
| for (const label of existingLabelsToRemove) { | ||
| await github.rest.issues.removeLabel({ | ||
|
|
||
| if (workflowRun.pull_requests && workflowRun.pull_requests.length > 0) { | ||
| const issue_number = workflowRun.pull_requests[0].number; | ||
|
|
||
| console.log(`Processing PR #${issue_number} in repository '${owner}/${repo}'.`); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for API calls
The workflow run retrieval lacks try-catch error handling, and assumes the first PR is always the target PR.
Consider this enhanced implementation:
- const { data: workflowRun } = await github.rest.actions.getWorkflowRun({
- owner,
- repo,
- run_id: workflowRunId,
- });
+ try {
+ const { data: workflowRun } = await github.rest.actions.getWorkflowRun({
+ owner,
+ repo,
+ run_id: workflowRunId,
+ });
+
+ if (!workflowRun.pull_requests?.length) {
+ throw new Error('No associated pull requests found');
+ }
+
+ // Verify we're targeting the correct PR
+ const targetPR = workflowRun.pull_requests.find(pr =>
+ pr.head.sha === context.payload.workflow_run.head_sha
+ );
+
+ if (!targetPR) {
+ throw new Error('Could not find matching PR for this workflow run');
+ }
+
+ const issue_number = targetPR.number;
+ } catch (error) {
+ core.setFailed(`Failed to process workflow run: ${error.message}`);
+ throw error;
+ }📝 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
| const workflowRunId = context.payload.workflow_run.id; | |
| const { data: workflowRun } = await github.rest.actions.getWorkflowRun({ | |
| owner, | |
| repo, | |
| issue_number, | |
| run_id: workflowRunId, | |
| }); | |
| // Filter the labels that exist on the PR and are in the labelsToRemove list | |
| const existingLabelsToRemove = labels | |
| .filter(label => labelsToRemove.includes(label.name)) | |
| .map(label => label.name); | |
| // Remove each label | |
| for (const label of existingLabelsToRemove) { | |
| await github.rest.issues.removeLabel({ | |
| if (workflowRun.pull_requests && workflowRun.pull_requests.length > 0) { | |
| const issue_number = workflowRun.pull_requests[0].number; | |
| console.log(`Processing PR #${issue_number} in repository '${owner}/${repo}'.`); | |
| const workflowRunId = context.payload.workflow_run.id; | |
| try { | |
| const { data: workflowRun } = await github.rest.actions.getWorkflowRun({ | |
| owner, | |
| repo, | |
| run_id: workflowRunId, | |
| }); | |
| if (!workflowRun.pull_requests?.length) { | |
| throw new Error('No associated pull requests found'); | |
| } | |
| // Verify we're targeting the correct PR | |
| const targetPR = workflowRun.pull_requests.find(pr => | |
| pr.head.sha === context.payload.workflow_run.head_sha | |
| ); | |
| if (!targetPR) { | |
| throw new Error('Could not find matching PR for this workflow run'); | |
| } | |
| const issue_number = targetPR.number; | |
| console.log(`Processing PR #${issue_number} in repository '${owner}/${repo}'.`); |
Comment on lines
+45
to
+78
| const { data: labels } = await github.rest.issues.listLabelsOnIssue({ | ||
| owner, | ||
| repo, | ||
| issue_number, | ||
| name: label, | ||
| }); | ||
| console.log(`Label '${label}' has been removed.`); | ||
| } | ||
|
|
||
| if (existingLabelsToRemove.length === 0) { | ||
| console.log('No matching labels found to remove.'); | ||
| } | ||
| github.rest.issues.addLabels({ | ||
|
|
||
| const existingLabelsToRemove = labels | ||
| .filter(label => labelsToRemove.includes(label.name)) | ||
| .map(label => label.name); | ||
|
|
||
| for (const label of existingLabelsToRemove) { | ||
| await github.rest.issues.removeLabel({ | ||
| owner, | ||
| repo, | ||
| issue_number, | ||
| name: label, | ||
| }); | ||
| console.log(`Label '${label}' has been removed.`); | ||
| } | ||
|
|
||
| if (existingLabelsToRemove.length === 0) { | ||
| console.log('No matching labels found to remove.'); | ||
| } | ||
|
|
||
| await github.rest.issues.addLabels({ | ||
| owner, | ||
| repo, | ||
| issue_number, | ||
| labels: ['approved','auto merge'] | ||
| labels: ['approved', 'auto merge'], | ||
| }); | ||
| console.log(`Labels 'approved' and 'auto merge' added.`); | ||
| } else { | ||
| throw new Error('No associated pull requests found for this workflow run.'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve label management robustness and maintainability
- Label names are hard-coded in multiple places
- No validation before adding new labels
- Label operations could partially fail
Consider this more robust implementation:
+ // Define labels configuration at the top
+ const LABELS_CONFIG = {
+ remove: ['waiting-review', 'ready to review'],
+ add: ['approved', 'auto merge']
+ };
+
+ async function validateLabels(github, owner, repo, labels) {
+ const { data: repoLabels } = await github.rest.issues.listLabelsForRepo({
+ owner,
+ repo,
+ });
+ const validLabels = repoLabels.map(l => l.name);
+ const invalidLabels = labels.filter(l => !validLabels.includes(l));
+ if (invalidLabels.length) {
+ throw new Error(`Invalid labels: ${invalidLabels.join(', ')}`);
+ }
+ }
+
try {
+ // Validate all labels exist in repository
+ await validateLabels(github, owner, repo, [
+ ...LABELS_CONFIG.remove,
+ ...LABELS_CONFIG.add
+ ]);
+
const { data: labels } = await github.rest.issues.listLabelsOnIssue({
owner,
repo,
issue_number,
});
const existingLabelsToRemove = labels
.filter(label => labelsToRemove.includes(label.name))
.map(label => label.name);
- for (const label of existingLabelsToRemove) {
- await github.rest.issues.removeLabel({
+ // Remove labels in parallel
+ await Promise.all(existingLabelsToRemove.map(label =>
+ github.rest.issues.removeLabel({
owner,
repo,
issue_number,
name: label,
- });
- console.log(`Label '${label}' has been removed.`);
- }
+ }).then(() => console.log(`Label '${label}' removed`))
+ .catch(e => console.warn(`Failed to remove label '${label}': ${e.message}`))
+ ));
if (existingLabelsToRemove.length === 0) {
console.log('No matching labels found to remove.');
}
- await github.rest.issues.addLabels({
+ // Add new labels
+ const result = await github.rest.issues.addLabels({
owner,
repo,
issue_number,
- labels: ['approved', 'auto merge'],
+ labels: LABELS_CONFIG.add,
});
- console.log(`Labels 'approved' and 'auto merge' added.`);
+ console.log(`Labels added: ${LABELS_CONFIG.add.join(', ')}`);
+ } catch (error) {
+ core.setFailed(`Label management failed: ${error.message}`);
+ throw error;
}Consider moving the label configuration to a repository-level configuration file (e.g., .github/label-workflow.yml) for better maintainability.
📝 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
| const { data: labels } = await github.rest.issues.listLabelsOnIssue({ | |
| owner, | |
| repo, | |
| issue_number, | |
| name: label, | |
| }); | |
| console.log(`Label '${label}' has been removed.`); | |
| } | |
| if (existingLabelsToRemove.length === 0) { | |
| console.log('No matching labels found to remove.'); | |
| } | |
| github.rest.issues.addLabels({ | |
| const existingLabelsToRemove = labels | |
| .filter(label => labelsToRemove.includes(label.name)) | |
| .map(label => label.name); | |
| for (const label of existingLabelsToRemove) { | |
| await github.rest.issues.removeLabel({ | |
| owner, | |
| repo, | |
| issue_number, | |
| name: label, | |
| }); | |
| console.log(`Label '${label}' has been removed.`); | |
| } | |
| if (existingLabelsToRemove.length === 0) { | |
| console.log('No matching labels found to remove.'); | |
| } | |
| await github.rest.issues.addLabels({ | |
| owner, | |
| repo, | |
| issue_number, | |
| labels: ['approved','auto merge'] | |
| labels: ['approved', 'auto merge'], | |
| }); | |
| console.log(`Labels 'approved' and 'auto merge' added.`); | |
| } else { | |
| throw new Error('No associated pull requests found for this workflow run.'); | |
| } | |
| // Define labels configuration at the top | |
| const LABELS_CONFIG = { | |
| remove: ['waiting-review', 'ready to review'], | |
| add: ['approved', 'auto merge'] | |
| }; | |
| async function validateLabels(github, owner, repo, labels) { | |
| const { data: repoLabels } = await github.rest.issues.listLabelsForRepo({ | |
| owner, | |
| repo, | |
| }); | |
| const validLabels = repoLabels.map(l => l.name); | |
| const invalidLabels = labels.filter(l => !validLabels.includes(l)); | |
| if (invalidLabels.length) { | |
| throw new Error(`Invalid labels: ${invalidLabels.join(', ')}`); | |
| } | |
| } | |
| try { | |
| // Validate all labels exist in repository | |
| await validateLabels(github, owner, repo, [ | |
| ...LABELS_CONFIG.remove, | |
| ...LABELS_CONFIG.add | |
| ]); | |
| const { data: labels } = await github.rest.issues.listLabelsOnIssue({ | |
| owner, | |
| repo, | |
| issue_number, | |
| }); | |
| const existingLabelsToRemove = labels | |
| .filter(label => labelsToRemove.includes(label.name)) | |
| .map(label => label.name); | |
| // Remove labels in parallel | |
| await Promise.all(existingLabelsToRemove.map(label => | |
| github.rest.issues.removeLabel({ | |
| owner, | |
| repo, | |
| issue_number, | |
| name: label, | |
| }).then(() => console.log(`Label '${label}' removed`)) | |
| .catch(e => console.warn(`Failed to remove label '${label}': ${e.message}`)) | |
| )); | |
| if (existingLabelsToRemove.length === 0) { | |
| console.log('No matching labels found to remove.'); | |
| } | |
| // Add new labels | |
| const result = await github.rest.issues.addLabels({ | |
| owner, | |
| repo, | |
| issue_number, | |
| labels: LABELS_CONFIG.add, | |
| }); | |
| console.log(`Labels added: ${LABELS_CONFIG.add.join(', ')}`); | |
| } catch (error) { | |
| core.setFailed(`Label management failed: ${error.message}`); | |
| throw error; | |
| } |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1346 +/- ##
=======================================
Coverage 20.14% 20.14%
=======================================
Files 434 434
Lines 54724 54724
=======================================
Hits 11024 11024
Misses 43700 43700 ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Which Issue(s) This PR Fixes(Closes)
Fixes #1345
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
Bug Fixes
Refactor
Chores