-
Notifications
You must be signed in to change notification settings - Fork 170
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 new makefile targets for go mod verification #3550
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,9 @@ AUTOREST_IMAGE = quay.io/openshift-on-azure/autorest:${AUTOREST_VERSION} | |
GATEKEEPER_VERSION = v3.15.1 | ||
GOTESTSUM = gotest.tools/[email protected] | ||
|
||
# Golang version go mod tidy compatibility | ||
GOLANG_VERSION ?= 1.20 | ||
|
||
ifneq ($(shell uname -s),Darwin) | ||
export CGO_CFLAGS=-Dgpgme_off_t=off_t | ||
endif | ||
|
@@ -334,10 +337,23 @@ admin.kubeconfig: | |
aks.kubeconfig: | ||
hack/get-admin-aks-kubeconfig.sh | ||
|
||
.PHONY: go-tidy | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why each individual PHONY and not added to the overall PHONY at the bottom of the file? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean why we need to create three targets (with one PHONY per target) instead of one target (and one PHONY)? Each go module command has a different purpose, and by separating them, I believe, we get better code readability and flow of the targets. They could be grouped together as they are used today, but separation empowers more flexibility :) Having said, I don't have a strong opinion on that. |
||
go-tidy: # Run go mod tidy - add missing and remove unused modules. | ||
go mod tidy -compat=${GOLANG_VERSION} | ||
|
||
.PHONY: go-vendor | ||
go-vendor: # Run go mod vendor - only modules that are used in the source code will be vendored in (make vendored copy of dependencies). | ||
go mod vendor | ||
|
||
.PHONY: go-verify | ||
go-verify: go-tidy go-vendor # Run go mod verify - verify dependencies have expected content | ||
go mod verify | ||
|
||
.PHONY: vendor | ||
vendor: | ||
# See comments in the script for background on why we need it | ||
hack/update-go-module-dependencies.sh | ||
$(MAKE) go-verify | ||
|
||
.PHONY: install-go-tools | ||
install-go-tools: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,5 +41,3 @@ done | |
|
||
go get -u ./... | ||
|
||
go mod tidy -compat=1.20 | ||
go mod vendor |
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.
Should we specify 1.20.12? (Still finding my feet with Go, I have no idea how strict it is with versions)
For reference: #3548
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.
go mod tidy is expecting an x.y version, thus we can't do that.
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.
Let's just double check this is the version we need. I remember some CVE issues so even though we just moved to 1.20 we need to make sure we have the version with the xnet fixes in.
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.
Go 1.20 is the best version we can support right now, we'll definitely need to move to 1.22 soon though. My understanding is that we have many underlying components we have to upgrade as well if we do, so 1.20 min is our first best step towards compliance.
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.
Also 1.20 is what we already have in our scripts (this PR is not changing it). IMO, if we need to update it, it would be better to do in a specific PR for that purpose.