Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Soft fail in cases where label is not present #1473

Merged
merged 3 commits into from
Oct 20, 2023
Merged

Soft fail in cases where label is not present #1473

merged 3 commits into from
Oct 20, 2023

Conversation

trajan0x
Copy link
Contributor

@trajan0x trajan0x commented Oct 20, 2023

#1466 introduced a regression where if label is not present, ./.github/actions/remove-label will exit w/ status code 1. This is problematic since in packages.yml, we always try to remove the label (see this failure on #472 for example):

image

Summary by CodeRabbit

  • Improvement: Enhanced the 'Remove Label' action in our GitHub workflow. The update provides better error handling and more informative feedback when a label is not found on an issue or pull request. This change will make it easier for team members to understand the status of label removal tasks, improving the overall workflow management experience.

@github-actions github-actions bot added M-ci Module: CI size/xs labels Oct 20, 2023
@trajan0x trajan0x marked this pull request as ready for review October 20, 2023 18:03
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 20, 2023

Walkthrough

The changes primarily focus on enhancing error handling and feedback in the GitHub Actions workflow for removing labels. The code now uses asynchronous functions and provides more informative logging, especially when a label is not found on an issue or pull request.

Changes

File Summary
.github/actions/remove-label/action.yml The action's description is updated with escaped space characters. The JavaScript code now uses an async function and a try-catch block for improved error handling. Logging is added for cases when the label does not exist on the issue.

🐇

In the land of code, where the shadows lie,

An action tweaked, under GitHub's sky.

No label found? Fear not the quest,

For better logging now handles this test.

Async functions, try-catch in sight,

CodeRabbit hops, into the night. 🌙


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.

CodeRabbit Commands

  • Enter @coderabbitai pause as a PR comment to pause the reviews on a PR.
  • Enter @coderabbitai resume as a PR comment to resume the paused reviews.
  • For on-demand reviews, enter @coderabbitai review as a PR comment to trigger a review. This is useful when reviews are automatic disabled at the repository level.
  • Enter @coderabbitai: ignore in the PR description to stop a specific PR from being reviewed.

Note these commands work only in the PR comment. For conversations with the bot, please use the review comments on code diffs or files.

@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9a2aa73) 50.69204% compared to head (89b15ab) 50.69204%.
Report is 1 commits behind head on master.

❗ Current head 89b15ab differs from pull request most recent head 36b4533. Consider uploading reports for the commit 36b4533 to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##              master       #1473   +/-   ##
=============================================
  Coverage   50.69204%   50.69204%           
=============================================
  Files            356         356           
  Lines          24276       24276           
  Branches         268         268           
=============================================
  Hits           12306       12306           
  Misses         10775       10775           
  Partials        1195        1195           
Flag Coverage Δ
packages 90.62003% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between 410e055 and 89b15ab.
Files selected for processing (1)
  • .github/actions/remove-label/action.yml (2 hunks)
Additional comments (Suppressed): 2
.github/actions/remove-label/action.yml (2)
  • 2-2: The description of the action has been changed from "Remove a label to the issue or pull request" to "Remove\ a label to the issue or pull request". The backslash before the space seems unnecessary and might be a typo. Please verify.
- description: Remove\ a label to the issue or pull request
+ description: Remove a label from the issue or pull request
  • 38-56: The code has been refactored to use an async function and a try-catch block for improved error handling. This is a good practice as it allows for better control over the flow of the program and makes it easier to handle errors. The logging for cases when the label does not exist on the issue is a good addition as it provides better feedback in such scenarios. However, the function removeLabel is declared and immediately invoked. It would be more readable and maintainable to directly execute the code without wrapping it in a function.

@trajan0x trajan0x merged commit 245903a into master Oct 20, 2023
@trajan0x trajan0x deleted the fix/remove branch October 20, 2023 18:07
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 1

Commits Files that changed from the base of the PR and between 89b15ab and 36b4533.
Files selected for processing (1)
  • .github/actions/remove-label/action.yml (1 hunks)
Additional comments (Suppressed): 2
.github/actions/remove-label/action.yml (2)
  • 44-44: The label name is now passed as a string instead of an array. This change is correct as the removeLabel method expects a string for the label name. However, please verify that this change does not affect other parts of the code that may be expecting an array.

  • 48-52: The error handling has been improved to provide more specific feedback. If the label does not exist on the issue (status code 404), a log message is generated. For other errors, the error message is logged. This is a good practice as it provides more detailed feedback and makes debugging easier.

Comment on lines +38 to +54
async function removeLabel() {
try {
await github.rest.issues.removeLabel({
issue_number: ${{ steps.resolve_issue_number.outputs.issue }},
owner: context.repo.owner,
repo: context.repo.repo,
name: '${{ inputs.label }}'
});
} catch (error) {
// Log the error if it's about the label not being present.
if (error.status === 404) {
console.log(`The label '${{ inputs.label }}' does not exist on the issue.`);
} else {
console.error('Unexpected error:', error);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new hunk introduces an async function removeLabel to handle the label removal process. This function includes a try-catch block to handle errors, which is a good practice for error handling. However, the function is not awaited when called on line 56, which could lead to unhandled promise rejections if an error occurs. To fix this, you should use an immediately invoked function expression (IIFE) to allow awaiting the removeLabel function.

-           removeLabel();
+           (async () => {
+             await removeLabel();
+           })();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-ci Module: CI size/xs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant