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 license scan for pull requests #2299

Merged

Conversation

killianmuldoon
Copy link
Contributor

Adds a licence scan inline with what is currently being added in the core Cluster API repo with kubernetes-sigs/cluster-api#9184

Part of kubernetes-sigs/cluster-api#9184

Signed-off-by: killianmuldoon <kmuldoon@vmware.com>
Copy link
Contributor Author

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

/hold

Ensure the PR over on the CAPI repo is merged before this one to avoid parallel changes. kubernetes-sigs/cluster-api#9184

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 29, 2023
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
/approve

@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: e1604cdbde0497b95730cb2f2c86a5a3fffa86ac

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2023
TRIVY_OS="macOS"
fi

VERSION=${1}
GO_ARCH="$(go env GOARCH)"
Copy link
Member

Choose a reason for hiding this comment

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

Interesting that we don't have this line in core CAPI and probably just assume that GOARCH is set?
Maybe something to fix in core CAPI? (@killianmuldoon)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The GO_ARCH here is only used to give the resulting image a name - I'm not sure if it's used for anything else at this point - the ensure-trivy script sets its own GO_ARCH.

Maybe it's a good idea to just remove this here?

Copy link
Member

@sbueringer sbueringer Aug 30, 2023

Choose a reason for hiding this comment

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

I think it's not used to give it a name, it's used to match the name that is used by make docker-build.

I think if we drop this line we either:

  • have to pass GO_ARCH into the script
  • have to find another way to match it correctly (I guess GOARCH might be different on macOS vs Linux (?))

I would avoid implicitly depending on an env variable being exported by someone calling that script (which I think is what is happening in core CAPI now). (Because this makes it harder to call the script standalone and just a bit too magical/implicit for my taste)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right - I'll do a PR into CAPI to make them consistent and make CAPI's use of the GO_ARCH more explicit.

@sbueringer
Copy link
Member

Thx!

Just one nit for core CAPI (also commented on the core CAPI PR)

/approve
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi, 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:
  • OWNERS [chrischdi,sbueringer]

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

@k8s-ci-robot k8s-ci-robot merged commit 61d0956 into kubernetes-sigs:main Aug 30, 2023
17 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.9 milestone Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants