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

ebickle commented 3 months ago

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:

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
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 commented 3 months ago

A rough example of the PR is available here: https://github.com/actions/dependency-review-action/compare/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 commented 3 months ago

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 commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

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?

actions / dependency-review-action

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