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.

Sign up for GitHub

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

Jump to bottom

🌱 Add licence-scan for pull requests #9184

Merged
merged 1 commit into from
Aug 30, 2023
Merged

🌱 Add licence-scan for pull requests #9184

merged 1 commit into from
Aug 30, 2023

Conversation

killianmuldoon
Copy link
Contributor

This adds a trivy license scan to the Makefile verify target which runs on all pull requests. This should allow detection of undesired license changes in go dependencies.

Related to #9181

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label labels Aug 14, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 14, 2023
@killianmuldoon killianmuldoon mentioned this pull request Aug 14, 2023
Closed
3 tasks
@killianmuldoon
Copy link
Contributor Author

/area ci

@k8s-ci-robot k8s-ci-robot added area/ci Issues or PRs related to ci and removed do-not-merge/needs-area PR is missing an area label labels Aug 14, 2023
vincepri
vincepri reviewed Aug 14, 2023
View reviewed changes
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@killianmuldoon
Copy link
Contributor Author

/cherry-pick release-1.5

@k8s-infra-cherrypick-robot
Copy link

@killianmuldoon: once the present PR merges, I will cherry-pick it on top of release-1.5 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@killianmuldoon
Copy link
Contributor Author

/cherry-pick release-1.4

@k8s-infra-cherrypick-robot
Copy link

@killianmuldoon: once the present PR merges, I will cherry-pick it on top of release-1.4 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

furkatgofurov7
furkatgofurov7 reviewed Aug 15, 2023
View reviewed changes
hack/verify-licenses.sh Outdated Show resolved Hide resolved
hack/ensure-trivy.sh Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2023
chrischdi
chrischdi reviewed Aug 16, 2023
View reviewed changes
Makefile
TRACE=$(TRACE) ./hack/verify-container-images.sh $(TRIVY_VER)

.PHONY: verify-licenses
verify-licenses: ## Verify licenses
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
verify-licenses: ## Verify licenses
verify-licenses: $(TRIVY) ## Verify licenses

Should we start running ensure-trivy.sh as a separate target to fetch the binary (including the versioned binary + symlink to the version) as we do for go install based tools?

Copy link
Member

Choose a reason for hiding this comment

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

note: I could also take this as a follow-up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure given it's just a download - I'm fine either way though.

Copy link
Member

Choose a reason for hiding this comment

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

Would be a nice follow-up

Copy link
Member

Choose a reason for hiding this comment

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

Now that I reviewed the PR. I'm fine with the current version. I think the most important part is that we always use the right version specified in the Makefile (which we do)

Would be still nice to follow-up for consistency though

hack/ensure-trivy.sh Show resolved Hide resolved
hack/ensure-trivy.sh Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2023
@killianmuldoon killianmuldoon force-pushed the pr-add-license-scan branch 2 times, most recently from 289fec5 to 864dba4 Compare August 24, 2023 16:05
@killianmuldoon killianmuldoon changed the title 🌱 Add licence-scan for pull requests [WIP] 🌱 Add licence-scan for pull requests Aug 24, 2023
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 24, 2023
@killianmuldoon killianmuldoon force-pushed the pr-add-license-scan branch 3 times, most recently from fc68b01 to b891221 Compare August 24, 2023 16:41
@killianmuldoon killianmuldoon changed the title [WIP] 🌱 Add licence-scan for pull requests 🌱 Add licence-scan for pull requests Aug 24, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 24, 2023
killianmuldoon
killianmuldoon commented Aug 24, 2023
View reviewed changes
hack/verify-licenses.sh Outdated Show resolved Hide resolved
hack/verify-licenses.sh Show resolved Hide resolved
@killianmuldoon
Copy link
Contributor Author

Current state:

  • Updated this PR to match the exact guidance of the CNCF upstream.
  • Added a specific exclude for hashicorp/hcl which we should probably move toward removing. Issue Review hashicorp license change impact #9181
  • Using trivy as it's simpler - i.e. can be run locally with no API - than snyk or fossa which are also used in the community. k/k uses a few custom scripts, and we don't have full guidance for how to scan from upstream yet.

@killianmuldoon killianmuldoon mentioned this pull request Aug 24, 2023
Closed
3 tasks
sbueringer
sbueringer reviewed Aug 29, 2023
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Nice work. Thank you very much for working on this!

hack/verify-licenses.sh Show resolved Hide resolved
Makefile
TRACE=$(TRACE) ./hack/verify-container-images.sh $(TRIVY_VER)

.PHONY: verify-licenses
verify-licenses: ## Verify licenses
Copy link
Member

Choose a reason for hiding this comment

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

Now that I reviewed the PR. I'm fine with the current version. I think the most important part is that we always use the right version specified in the Makefile (which we do)

Would be still nice to follow-up for consistency though

hack/verify-container-images.sh Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
hack/verify-licenses.sh Show resolved Hide resolved
@killianmuldoon killianmuldoon force-pushed the pr-add-license-scan branch 2 times, most recently from c173312 to 294267b Compare August 29, 2023 12:32
@sbueringer
Copy link
Member

Last nit: #9184 (comment)

@killianmuldoon
Add licence-scan for pull requests
291ea46 
Signed-off-by: killianmuldoon <kmuldoon@vmware.com>
@sbueringer
Copy link
Member

Thank you!

/approve

/assign @chrischdi

@sbueringer sbueringer added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@killianmuldoon killianmuldoon mentioned this pull request Aug 29, 2023
Merged
chrischdi
chrischdi reviewed Aug 30, 2023
View reviewed changes
Copy link
Member

@chrischdi chrischdi left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 1cf49f28629713a5187694d08d8b969f3cb8317b

@k8s-ci-robot k8s-ci-robot merged commit 613a37e into kubernetes-sigs:main Aug 30, 2023
23 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.6 milestone Aug 30, 2023
@k8s-infra-cherrypick-robot
Copy link

@killianmuldoon: #9184 failed to apply on top of branch "release-1.5":

Applying: Add licence-scan for pull requests
.git/rebase-apply/patch:240: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M	CONTRIBUTING.md
M	Makefile
Falling back to patching base and 3-way merge...
Auto-merging Makefile
CONFLICT (content): Merge conflict in Makefile
Auto-merging CONTRIBUTING.md
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add licence-scan for pull requests
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-infra-cherrypick-robot
Copy link

@killianmuldoon: #9184 failed to apply on top of branch "release-1.4":

Applying: Add licence-scan for pull requests
.git/rebase-apply/patch:240: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M	CONTRIBUTING.md
M	Makefile
M	hack/verify-container-images.sh
Falling back to patching base and 3-way merge...
Auto-merging hack/verify-container-images.sh
CONFLICT (content): Merge conflict in hack/verify-container-images.sh
Auto-merging Makefile
CONFLICT (content): Merge conflict in Makefile
Auto-merging CONTRIBUTING.md
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add licence-scan for pull requests
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sbueringer
sbueringer reviewed Aug 30, 2023
View reviewed changes
hack/verify-container-images.sh
@@ -22,48 +22,25 @@ if [[ "${TRACE-0}" == "1" ]]; then
set -o xtrace
fi

TRIVY_VERSION=0.34.0
VERSION=${1}

Copy link
Member

Choose a reason for hiding this comment

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

@killianmuldoon Found in https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/pull/2299/files. We probably still want to set GO_ARCH

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrischdi chrischdi chrischdi left review comments

@vincepri vincepri vincepri left review comments

@sbueringer sbueringer sbueringer left review comments

@furkatgofurov7 furkatgofurov7 furkatgofurov7 left review comments

@fabriziopandini fabriziopandini Awaiting requested review from fabriziopandini

@jackfrancis jackfrancis Awaiting requested review from jackfrancis

Assignees

@chrischdi chrischdi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/ci Issues or PRs related to ci cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@killianmuldoon @k8s-infra-cherrypick-robot @sbueringer @k8s-ci-robot @chrischdi @vincepri @fabriziopandini @furkatgofurov7