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

Add comment when warn-only: true and comment-summary-in-pr: on-failure #817

Open
ebickle opened this issue Aug 19, 2024 · 7 comments · May be fixed by #827
Open

Add comment when warn-only: true and comment-summary-in-pr: on-failure #817

ebickle opened this issue Aug 19, 2024 · 7 comments · May be fixed by #827
Labels
enhancement New feature or request

Comments

@ebickle
Copy link

ebickle commented Aug 19, 2024

I'm looking to deploy dependency-review-action using a required workflow to ensure that all developers are aware of dependency issues when pull requests are opened.

Since the required workflow rule will be applied to every repository:

  1. We don't want the workflow to fail with an error when a dependency issue is found, only report on them so developers are informed
  2. We don't want a dependency review summary comment added to every pull request, only ones with issues

The action input warn-only: true disables errors. The action input comment-summary-in-pr: on-failure avoids adding a comment on every pull request. However, they are used at the same time, no comment summary is added. This is because the comment summary is only added if the entire action will fail with an exit code (core.setFailure) not if a dependency review "rule" fails.

Would you be open to having me submit a pull request for the following?

  • Modify comment-summary-in-pr: on-failure so that a comment is added if any dependency review failure occurs, even if warn-only: true is set.

In addition, if you're open to additional changes, I'd like to open a second pull request to have dependency review optionally create a pull request status check that is independent of the workflow. This would allow the dependency review action to work similarly to CodeQL - the success of the action itself would be (optionally) independent from the "dependency failures", allowing developers to enforce that dependency review is "run" but still have granular control over whether or not the dependency check "report" itself is enforced. This would be a more involved PR, so I'd like to do it after the 'quick fix' to comment-summary-in-pr: on-failure.

@ebickle
Copy link
Author

ebickle commented Aug 19, 2024

A rough example of the PR is available here: main...ebickle:dependency-review-action:fix/comment-warn-only. Note that the total number of failures is bubbled up to the run() function to facilitate the future addition of a PR check.

@jonjanego
Copy link
Collaborator

Hi @ebickle !

Modify comment-summary-in-pr: on-failure so that a comment is added if any dependency review failure occurs, even if warn-only: true is set.

just so that i understand, you want to do it this way instead of using comment-summary-in-pr: always so that you see messages in the PR only for failures of the criteria you've set?

@jonjanego jonjanego added the enhancement New feature or request label Aug 23, 2024
@jonjanego
Copy link
Collaborator

re:

I'd like to open a second pull request to have dependency review optionally create a pull request status check that is independent of the workflow. This would allow the dependency review action to work similarly to CodeQL - the success of the action itself would be (optionally) independent from the "dependency failures", allowing developers to enforce that dependency review is "run" but still have granular control over whether or not the dependency check "report" itself is enforced. This would be a more involved PR, so I'd like to do it after the 'quick fix' to comment-summary-in-pr: on-failure.

I love this idea! lets discuss it in a separate issue, however, just to keep this one focused

@ebickle
Copy link
Author

ebickle commented Aug 30, 2024

just so that i understand, you want to do it this way instead of using comment-summary-in-pr: always so that you see messages in the PR only for failures of the criteria you've set?

That's correct!

I'm looking at deploying the action in a required workflow for thousands of repositories. To reduce developer impact during the rollout and ongoing operation the idea is to have the workflow succeed (warn-only: true) and reduce the "noise" shown to developers so that only important things are commented on the PR (comment-summary-in-pr: on-failure).

I've since moved to an approach that adds a commit check instead so it's not something I directly need any longer, but it seems like a valuable change that I'm happy to toss a PR in for.

@jonjanego
Copy link
Collaborator

I've since moved to an approach that adds a commit check instead so it's not something I directly need any longer, but it seems like a valuable change that I'm happy to toss a PR in for.

Please feel free to. It may take the team a little bit to get to a review, but it seems like a useful change to have.

@benjamin-scc
Copy link

I'm looking at deploying the action in a required workflow for thousands of repositories. To reduce developer impact during the rollout and ongoing operation the idea is to have the workflow succeed (warn-only: true) and reduce the "noise" shown to developers so that only important things are commented on the PR (comment-summary-in-pr: on-failure).

+1

I'm also looking at integrating this action into a reusable workflow used by hundreds of repositories. The ideal initial rollout would be to only warn about failures (in the form of a PR comment) while the workflow succeeds; in a bid to keep the noise and toil down for developers, while at the same time making them aware of security issues.

@vibha-bd
Copy link

vibha-bd commented Oct 4, 2024

Just to add this into the same conversation, as I too agree that it should comment only when it detects a failure, when we set [comment](comment-summary-in-pr: on-failure), and if the dependency_review action is running on a PR, then it does not remove the comment on the PR when the Vuln is fixed.
Scenario:

  • PR is raised with Vuln code.
  • Dependency_review comments on PR with detected Vuln.
  • Dev team fixes the code, refactors the code and updates the PR
  • Dependency_review action now does not update/remove the PR comment, because it is set to comment only when it detects a failure.

This leads to improper comments in PR. Quite an interesting enhancement to have. Can it be prioritised?

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

Successfully merging a pull request may close this issue.

4 participants