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

[ci] run dash-licenses as part of GH workflow #9953

Merged
merged 10 commits into from
Aug 27, 2021
Merged

Conversation

marcdumais-work
Copy link
Contributor

@marcdumais-work marcdumais-work commented Aug 23, 2021

What it does

Add a "3PP FOSS license check" GitHub workflow, that uses
the Eclipse Foundation's dash-licences tool to validate the
licenses of our npm dependencies.

One can also run the license check locally with command:
"yarn license:check"

I have considered 3 alternatives:

(1) add the license check as a step of the build job of the build workflow.

The license check step needs to report
success or it will fail the build job and the publish job
(when it applies). If the license check reports success, it's
burried in the multiple steps of its job, not discoverable
from the PR page.

(2) add the license check as a new job in the build workflow.

This works better but there is redundency in the job - we need
to re-install node, checkout the repo, etc.

One advantage of this approach is that the license check has
visibility in the PR's CI checks section and can be made to fail
without impacting the other jobs.

(3) (chosen) add a new workflow for the license check

Like option 2, this gives the license check visibility
in the PR's CI checks section and the job can fail without impacting
other workflows/jobs.

This option has the additional advantage that it's quite
generic (npm/yarn projects) and could be re-used more
easily, without changing the main workflow of the adopting repo.

How to test

  • Run the license check locally:
    yarn license:check and verify that ~38 dependencies are found that need attention
  • Look at this PR's CI steps and verify that 3PP License Check fails, and in the log, lists the same dependencies

Review checklist

Reminder for reviewers

@marcdumais-work marcdumais-work added ci issues related to CI / tests eclipse issues related to eclipse / eclipse foundation labels Aug 23, 2021
Add a "3PP FOSS license check" GitHub workflow, that uses
the Eclipse Foundation dash-licences tool to validate the
licenses of our npm dependencies.

One can also run the license check locally with command:
"yarn license:check"

I have considered 3 alternatives:

1) add the license check as a step of the build job of the
build workflow. The license check step needs to report
success or it will fail the build job and the publish job
(when it applies). If the license check reports success, it's
burried in the multiple steps of its job, not discoverable
from the PR page.

2) add the license check as a new job in the build workflow.
This works better but there is redundency in the job -  we need
to re-install node, checkout the repo, etc.

One advantage of this approach is that the license check has
visibility in the PR's CI checks section and can be made to fail
without impacting the other jobs.

3) (chosen) add a new workflow for the license check

Like option 2, this gives the license check visibility
in the PR's CI checks section and the job can fail without impacting
other workflows/jobs.

This option has the additional advantage that it's quite
generic (for npm/yarn projects) and could be re-used more
easily, without changing the main workflow of the adopting repo.

Signed-off-by: Marc Dumais <[email protected]>
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, just pushed a commit to improve the script a bit.

.github/workflows/license-check.yml Show resolved Hide resolved
@paul-marechal
Copy link
Member

Also how do we deal with the failing CI?

@paul-marechal paul-marechal force-pushed the dash-licenses branch 2 times, most recently from 15e23d6 to 55d652c Compare August 23, 2021 22:01
@marcdumais-work
Copy link
Contributor Author

Also how do we deal with the failing CI?

For now this is just informational. It's expected that the license check job will be shown as failed all the time. PRs that introduce new dependencies should still check them as per our process. In parallel, the dependencies that routinely fail the check are being investigated, and their number will slowly go down, eventually to zero (in theory).

As a follow-up, we could add a white-list of dependencies that are know to be ok for our project (or at least being investigated), that would be filtered-out.

@paul-marechal
Copy link
Member

As a follow-up, we could add a white-list of dependencies that are know to be ok for our project (or at least being investigated), that would be filtered-out.

I can see to implement this, because I am not fond of having a failed check on each and every PR from now on :)

@marcdumais-work
Copy link
Contributor Author

I can see to implement this, because I am not fond of having a failed check on each and every PR from now on :)

It does reflect reality though - not all that's listed is approved or being investigated ATM.

@paul-marechal
Copy link
Member

Agreed, but I think "unknown" status is different from flat out "dependency license is problematic" hence why I would only expect to fail because of the latter.

Copy link

@waynebeaton waynebeaton left a comment

Choose a reason for hiding this comment

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

LGTM

@paul-marechal
Copy link
Member

paul-marechal commented Aug 24, 2021

\o/ Woohoo the 3PP check is green!

I added a commit that implements a baseline: Even if dash-licenses fails, if what is reported is part of the baseline then we'll ignore its error code and exit normally.

The baseline is currently a map of entries (as reported by dash-licenses) to an optional data. My idea is to map each entry to its associated CQ (approved or not) to help keep track of what's what, as I currently have no idea if anything is already cleared.

@marcdumais-work I invite you to fill the baseline with URLs to relevant CQs for what you know about. This should help with understanding what's the state of each dependency defined in this baseline.

In the future as those dependencies get cleared or not, we shall remove entries from that baseline until it is emptied?

@paul-marechal
Copy link
Member

paul-marechal commented Aug 24, 2021

Note that I also tried to update the documentation about PR handling regarding this new check.

@marcdumais-work
Copy link
Contributor Author

In the future as those dependencies get cleared or not, we shall remove entries from that baseline until it is emptied?

yes, we can do that. I submitted a CQ recently that should clear all our production dependencies, so for the most part, the baseline is composed of lower-priority test/build dependencies.

Did a first pass, adding information in the `license-check-baseline.json` file.

Signed-off-by: Marc Dumais <[email protected]>
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

I know it's mostly my code now, but LGTM!

@marcdumais-work
Copy link
Contributor Author

In the future as those dependencies get cleared

One bonus feature could be to list those baseline dependencies that have not come-up in the current scan. It probably means they can be removed, e.g. when we next modify that file.

@paul-marechal
Copy link
Member

paul-marechal commented Aug 25, 2021

@marcdumais-work done.

Apparently there were updates as entries that I had to include in the baseline yesterday seem to not be reported today:

image

edit: I also notice that GitHub action just got mixed up when printing stdout and stderr...

It should look like this:

image

Weird quirk, but fine to me for now.

@paul-marechal
Copy link
Member

paul-marechal commented Aug 26, 2021

Will merge after the 1.17.0 release.

@paul-marechal paul-marechal merged commit 8e7f15b into master Aug 27, 2021
@paul-marechal paul-marechal deleted the dash-licenses branch August 27, 2021 02:36
@github-actions github-actions bot added this to the 1.18.0 milestone Aug 27, 2021
@marcdumais-work
Copy link
Contributor Author

marcdumais-work commented Aug 30, 2021

Apparently there were updates as entries that I had to include in the baseline yesterday seem to not be reported today:

I manually ran the license-check in the "auto-submit" mode, which knocked-off a few dependencies. I'll do that once in a while.

RomanNikitenko pushed a commit that referenced this pull request Sep 16, 2021
Add a "3PP FOSS license check" GitHub workflow that uses
the Eclipse Foundation dash-licences tool to validate the
licenses of our npm dependencies.

One can also run the license check locally with command:
"yarn license:check"

I have considered 3 alternatives:

1) Add the license check as a step of the build job of the
build workflow. The license check step needs to report
success or it will fail the build job and the publish job
(when it applies). If the license check reports success, it's
buried in the multiple steps of its job, not discoverable
from the PR page.

2) Add the license check as a new job in the build workflow.
This works better but there is redundancy in the job -  we need
to re-install node, checkout the repo, etc.

One advantage of this approach is that the license check has
visibility in the PR's CI checks section and can be made to fail
without impacting the other jobs.

3) _(chosen)_ Add a new workflow for the license check.

Like option 2 this gives the license check visibility in the PR's CI
checks section and the job can fail without impacting other
workflows/jobs.

This option has the additional benefit that it's quite generic
(for npm/yarn projects) and could be re-used more easily,
without changing the main workflow of the adopting repo.

---

It is worth noting that there are some dependencies that are
not yet completely cleared in this repository (which most likely
come from the time before Theia was an Eclipse project).

The tool exits with an error set because of those. The solution
implemented here is to have a "baseline" of dependencies
known to make `dash-licenses` trip, and to ignore the scan
error only if everything that is being reported is part of the
baseline.

This means that the GitHub Action check on PRs should only fail
if new "restricted" dependencies are introduced, which indicates
that actions must be taken, such as creating CQs.

Even when green it is worth checking the logs of the check since
it will report any dependency that is no longer required in the
baseline: This will happen once the various databases are
updated following the CQs we fill!

Signed-off-by: Marc Dumais <[email protected]>
Co-authored-by: Paul Marechal <[email protected]>
RomanNikitenko pushed a commit that referenced this pull request Sep 16, 2021
Add a "3PP FOSS license check" GitHub workflow that uses
the Eclipse Foundation dash-licences tool to validate the
licenses of our npm dependencies.

One can also run the license check locally with command:
"yarn license:check"

I have considered 3 alternatives:

1) Add the license check as a step of the build job of the
build workflow. The license check step needs to report
success or it will fail the build job and the publish job
(when it applies). If the license check reports success, it's
buried in the multiple steps of its job, not discoverable
from the PR page.

2) Add the license check as a new job in the build workflow.
This works better but there is redundancy in the job -  we need
to re-install node, checkout the repo, etc.

One advantage of this approach is that the license check has
visibility in the PR's CI checks section and can be made to fail
without impacting the other jobs.

3) _(chosen)_ Add a new workflow for the license check.

Like option 2 this gives the license check visibility in the PR's CI
checks section and the job can fail without impacting other
workflows/jobs.

This option has the additional benefit that it's quite generic
(for npm/yarn projects) and could be re-used more easily,
without changing the main workflow of the adopting repo.

---

It is worth noting that there are some dependencies that are
not yet completely cleared in this repository (which most likely
come from the time before Theia was an Eclipse project).

The tool exits with an error set because of those. The solution
implemented here is to have a "baseline" of dependencies
known to make `dash-licenses` trip, and to ignore the scan
error only if everything that is being reported is part of the
baseline.

This means that the GitHub Action check on PRs should only fail
if new "restricted" dependencies are introduced, which indicates
that actions must be taken, such as creating CQs.

Even when green it is worth checking the logs of the check since
it will report any dependency that is no longer required in the
baseline: This will happen once the various databases are
updated following the CQs we fill!

Signed-off-by: Marc Dumais <[email protected]>
Co-authored-by: Paul Marechal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci issues related to CI / tests eclipse issues related to eclipse / eclipse foundation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants