-
Notifications
You must be signed in to change notification settings - Fork 26
CI: add job for checking licenses from dependencies #741
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @flaviodsr ,
So comments worth checking.
As a side note, if you don't have your golang bin
directory in your path the wwhrd
execution fails
.wwhrd.yml
Outdated
@@ -0,0 +1,7 @@ | |||
allowlist: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename the file to have yaml
extension rather than yml
, as we already have files with the 2nd extesion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
.github/workflows/ci-cd.yaml
Outdated
- uses: actions/setup-go@v2 | ||
with: | ||
go-version: 1.16 | ||
- name: Install WWHRD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really need to run this in 2 steps, right?
check-licenses
already depend on install-wwhrd
, so calling the 2nd would call the 1st by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right, I just thought that in case of failure of the install it would be more clear having a step specifically doing that.
Anyways, I have removed the step.
|
||
.PHONY: install-wwhrd | ||
install-wwhrd: | ||
which wwhrd || go install github.com/frapposelli/wwhrd@latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that much about golang dependencies, but I wonder if there is some option to define this dependency as a development
dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not aware of such option. Did some research and could not find it either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would ask @fabriziosestito since he is the lord of that pattern but I think we can actually declare wwhrd
in the tools/tools.go
file having it fetched inside deps
0a8a6eb
to
fdfec60
Compare
@arbulu89 thanks for the review. Regarding the golang bin directory, do you have a suggestion on how to fix that? I could add |
8e3b3b0
to
8ecdbd4
Compare
The check is implemented using wwhrd [1] and the allowed licenses are listed on the `.wwhrd.yml` file. 1. https://github.com/frapposelli/wwhrd
8ecdbd4
to
4c31254
Compare
The check is implemented using wwhrd [1] and the allowed
licenses are listed on the
.wwhrd.yml
file.Fix: #349