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

fix: add summary comment on failure when warn-only: true #827

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ebickle
Copy link

@ebickle ebickle commented Sep 6, 2024

Fixes #817

Summary

When the parameter warn-only is set to true, allow a pull request comment to be created when comment-summary-in-pr is set to on-failure.

Testing

The action was tested with the following scenarios:

  1. New package with vulnerability in pull request, warn-only: true, comment-summary-in-pr: always
  2. New package with vulnerability in pull request, warn-only: true, comment-summary-in-pr: on-failure
  3. New package with vulnerability in pull request, warn-only: true, comment-summary-in-pr: never
  4. No changes to dependencies in pull request, warn-only: true, comment-summary-in-pr: always
  5. No changes to dependencies in pull request, warn-only: true, comment-summary-in-pr: on-failure
  6. No changes to dependencies in pull request, warn-only: true, comment-summary-in-pr: never

Unit tests have not been added since both main.ts and comment-pr.ts currently lack unit test files. I looked at adding unit tests for comment-pr.ts but the static octokit constructor (new retryingOctokit) made this extremely difficult due to jest.mock hoisting. To make the file more testable, retryingOctokit should be replaced with non-static logic, such as an update to octokitClient in utils.ts that would have an optional parameter to allow retries. This change would have been much larger and far beyond the scope of this pull request.

Implementation decisions

  • The fix requires the config.comment_summary_in_pr === 'on-failure' test in comment-pr.ts to no longer rely on a non-zero exit code. To signal a failure, a new parameter was added to the commentPr function.
  • A failureCount: number was used instead of a boolean to simplify the logic - it's much easier to incrementally do a failureCount += than to try to do boolean logic on the same line as function calls. In addition, failureCount seemed more useful for future action logging.
  • To be consistent with the rest of the action's functionality, the logging and setFailed within deny.ts was moved out into printDeniedDependencies.
  • The failureCount variable in main.ts can be used for other future functionality, such as an implementation of Add option for commit status check #825

@ebickle ebickle requested a review from a team as a code owner September 6, 2024 19:51
Copy link
Contributor

@elireisman elireisman left a comment

Choose a reason for hiding this comment

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

Thank for you contributing this change! Also really appreciate the manual testing, this functionality is obtuse to unit test at the moment. That too is being tracked, appreciate you working around it for now 🙇

I'd be partial to sticking with the bool flag for fails until we have time for a larger refactor, but no need to deal with that now since we're not relying on the returned count itself yet anywhere public facing 👍

Before landing this change, you'll need to update the PR (NPM dist packaging) but otherwise LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add comment when warn-only: true and comment-summary-in-pr: on-failure
2 participants