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 Vulnerabilities and license checks #184

Closed
wants to merge 2 commits into from

Conversation

tspascoal
Copy link
Contributor

New Feature

Creates two checks:

  • A check with vulnerabilities added. If the check fails, it adds to the check the list of vulnerabilities found grouped by manifest. The check fails if vulnerable package(s) are added.
  • License check. A license check is created and it lists on the detail incompatible licenses (if any) and the unknown licenses. The check fails if there are incompatible licenses and succeeds otherwise

By separating the failure in two, users can now control if license or vulnerabilities individually prevent the merge from happening.

Checks names are configurable.

Breaking Changes

A breaking change was introduced, the action no longer fails if there are vulnerabilities or license violations. fail-on-violation parameter can be set to true (default value is false) to maintain backward compatibility.

image

There is another potential breaking change, since the workflow now requires checks: write permission, might need to add the new permission explicitly. The dependency review starter workflow will also have to be updated to add this permission

Creates two checks:

- A check with vulnerabilities added. If the check fails, it adds to the
check the list of vulnerabilities found grouped by manifest. The check
fails if vulnerables packages are added.
- License check. A license check is created and it lists on the detail
incompatible licenses (if any) and the unknown licenses. The check fails
if there are incompatible licenses and succeeds otherwise

Checks names are configurable.

A breaking change was introduced, the action no longer fails if there are
vulnerabilities or license violations. fail-on-violation parameter
can be set to true to maintain backward compatibility.
@tspascoal tspascoal requested a review from a team as a code owner August 7, 2022 11:32
@febuiles
Copy link
Contributor

@tspascoal I love the idea of having one check per feature, but the main Action one should never be green if one of the children checks fails. Can we overcome this? If not I don't think we should be spending extra cycles here (it's backwards incompatible and adds extra write security permissions).

@tspascoal
Copy link
Contributor Author

@tspascoal I love the idea of having one check per feature, but the main Action one should never be green if one of the children checks fails. Can we overcome this? If not I don't think we should be spending extra cycles here (it's backwards incompatible and adds extra write security permissions).

@febuiles Not failing the action, allows extra flexibility since users can fine grain when the PR can't be merged by requiring the checks them want (one of them or both).

Backward compatibility (failing the action if one of the checks fails) can be achieved by explicitly setting the parameter fail-on-violation to true (default value is false).

This allows both the extra flexibility or getting the previous behavior with this parameter.

So getting the old behavior by default, is a matter of changing the default value for fail-on-violation or let the user control by enabling if they want it.

@febuiles
Copy link
Contributor

Backward compatibility (failing the action if one of the checks fails) can be achieved by explicitly setting the parameter fail-on-violation to true (default value is false).
This allows both the extra flexibility or getting the previous behavior with this parameter.

In order for this to be backwards compatible I'd expect it to behave the same way that it has without the user having to modify their YAML. Remember that we pin all release to v2 atm, so we can't release as part of this version because we'll change the behavior for the current users. We can release a v3, but we release major versions only for breaking backwards compatibility!

I don't know how the child checks work, could we default fail-on-violation to true? In that case only users who want this new behavior would be modifying their workflows. We'd be able to switch this behavior once v3 is released if that's the way we want to move forward (still not sure what the right decision is here, would like to hear back from users in case you have feedback!).

@tspascoal
Copy link
Contributor Author

tspascoal commented Aug 19, 2022

Backward compatibility (failing the action if one of the checks fails) can be achieved by explicitly setting the parameter fail-on-violation to true (default value is false).
This allows both the extra flexibility or getting the previous behavior with this parameter.

In order for this to be backwards compatible I'd expect it to behave the same way that it has without the user having to modify their YAML. Remember that we pin all release to v2 atm, so we can't release as part of this version because we'll change the behavior for the current users. We can release a v3, but we release major versions only for breaking backwards compatibility!

Yeah, my intention was to make it a V3, I've added a Upgrading from V2: Breaking changes section to the readme see Since the new permission is potentially a breaking change, might as well introduce a nicer semantic since users (may) have to change the workflow anyway.

I don't know how the child checks work, could we default fail-on-violation to true? In that case only users who want this new behavior would be modifying their workflows. We'd be able to switch this behavior once v3 is released if that's the way we want to move forward (still not sure what the right decision is here, would like to hear back from users in case you have feedback!).

If you feel that this should be a V2 release, we can certainly make fail-on-violation true by default (but users might still have to change the workflow due to permissions), and will need to update the readme to not mention the upgrade from V2.

Let me know which scenario you want to achieve:

  1. V2 with no breaking change on the action failure
  • (fail-on-violation will default to true)
  • No mention on readme to upgrade from V2, but we will still need to document the change of permission
  1. V3 with no breaking change on the action failure (fail-on-violation will default to true)
  • Upgrade from V2 will be tweaked
  1. V3 with breaking change on the action failure (fail-on-violation will default to false)

@febuiles
Copy link
Contributor

I keep forgetting about auth, thanks for bringing it up! We can wait until we have the new config file format before revisiting this.

@febuiles
Copy link
Contributor

The latest release of the action (v3) allows users to specify if they want to allow/disallow a specific check (vulns, licenses) in their runs. I know this is not exactly the same functionality this PR provided, but I feel it's close enough to close this for now.

@febuiles febuiles closed this Nov 11, 2022
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.

None yet

2 participants