Add Job Summary and PR comment

[Pradoxzon]

Description

Being able to view a summary of the PaC results as a job summary and as a comment when running in a PR, as opposed to the current error and warning annotations visible when you view the run of the action, would greatly improve the user experience for developers. This would be particularly useful for teams that use PaC as check on their PRs.

Potential Solution

Add the ability to enable PR comments with an argvs flag, similar to --display. Adding the flag would cause PaC to add a comment to the PR if it is running in a PR and have no effect otherwise.
The comment should include an indication of the status of the policy checks. For example, how many violations were found for each technology type. When the --display flag is enabled, it could also add information about the violations in a list or table format, allowing devs to easily identify the source(s) of the violations.

This would involve:

• Adding an argvs flag for enabling the PR comment, e.g. --pr-comment
• Tracking policy warnings and violations instead of just tracking the number of warnings and violations
• Building a summary of the results, either during the checks or afterwards
• Output the summary to both the Job Summary output (viewable in the Summary view of the workflow run) and as a comment in the PR (when the flag is enabled, and the action runs in a PR)

Would potentially include some changes to ghastoolkit to help enable these changes.

Examples

Example without --display enabled:

Example with --display enabled:

[GeekMasher]

@Pradoxzon I love this idea! Two main things on this. First is that I want to start moving away from argvs as this has become annoying for users.

The second is that this could be a feature for Policy v3 and set this in the policy file itself. I have a new block for displaying content to the user but only in the stdout versus anywhere else. This definitely could be a feature in there.

https://github.com/GeekMasherOrg/security-v3/blob/main/policy.yml#L4-L11

[Pradoxzon]

@GeekMasher I agree, moving away from argvs would be nice.

For implementation in Policy v3, the example you have in that linked file looks good. Though how would a user configure the policy if they want the PR comment but without the full list/breakdown of the alerts that caused violations? Like in the first example image above.

For the Job Summary output, my initial thought was to have it always enabled and just have it show more or less detail (e.g. the difference in the example images) based on the display flag. Without the display flag, it wouldn't show any details that aren't available by looking at the output of the workflow run, but it alleviates the need for users to dig through the log output.

Also, would it be ok if I submit a draft PR of my pass at adding this functionality to the current version of PaC? I already took a pass at adding it since I was looking to use the functionality for my users and got it to a working state. It could at least be used as a reference for implementing in v3.

[GeekMasher]

Though how would a user configure the policy if they want the PR comment but without the full list/breakdown of the alerts that caused violations?

Agree, maybe 2 settings?

• pr-summary for a summary (first example) and;
• pr-detailed for all details (second example).

display:
  pr-summary: false  # true by default, enabled
  pr-detailed: true  # false by default
  # if both are set, detailed takes priority

I'm up for other suggestions on this.

For the Job Summary output, my initial thought was to have it always enabled

I like this idea, I would have to have some setting to disable it as some users might not want it just pipeline breaking

Also, would it be ok if I submit a draft PR of my pass at adding this functionality to the current version of PaC?

Absolutely, if you are working on it for v2 then please continue. This was more of a heads up on the future of v3 when it comes out.

[Pradoxzon]

• pr-summary for a summary (first example) and;
• pr-detailed for all details (second example).

display:
  pr-summary: false
  pr-detailed: true
  # if both are set, detailed takes priority

I'm up for other suggestions on this

How about:

display:
  # Applies to both PR comments and the Job Summary
  detailed: true
  # Enables/disables the PR comment
  pr-summary: false

[GeekMasher]

• pr-summary for a summary (first example) and;
• pr-detailed for all details (second example).

display:
  pr-summary: false
  pr-detailed: true
  # if both are set, detailed takes priority

I'm up for other suggestions on this

How about:

display:
  # Applies to both PR comments and the Job Summary
  detailed: true
  # Enables/disables the PR comment
  pr-summary: false

I like it. Keeps it simple and if you want details, you would probably want it in both places.

[Pradoxzon]

Had a couple more thoughts from the conversation in #44 about if the PR comment should be default enabled/disabled.
My vote is to keep the feature as default disabled in v2.X, then change it to be default enabled in v3.

In v2, the PR comment feature was added in a minor/dot release, and it is a noticeable difference in behavior when enabled. Leaving it as default disabled allows users to decide to use the new feature only if they are aware of it and choose to enable it.

Whereas with v3 being a major release with a vast amount of other breaking changes, flipping the feature to default enable at this point would be generally easier for users to handle.