Add new makefile targets for go mod verification #3550
Conversation
razo7
requested review from
jewzaam,
bennerv,
hawkowl,
rogbas,
petrkotas,
jharrington22,
cblecker,
cadenmarchese,
UlrichSchlueter,
s-amann,
SudoBrendan,
Shivkumar13,
yjst2012,
jaitaiwan,
anshulvermapatel,
hlipsig and
tiguelu
as code owners
|
/azp run ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Everything looks good to me, except what @tiguelu already pointed about. Once thats changed. LGTM
| @@ -266,9 +269,22 @@ admin.kubeconfig: | |||
| aks.kubeconfig: | |||
| hack/get-admin-aks-kubeconfig.sh | |||
|
|
|||
| .PHONY: go-tidy | |||
There was a problem hiding this comment.
Why each individual PHONY and not added to the overall PHONY at the bottom of the file?
There was a problem hiding this comment.
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.
| @@ -18,6 +18,9 @@ GATEKEEPER_VERSION = v3.10.0 | |||
| GATEKEEPER_IMAGE ?= ${RP_IMAGE_ACR}.azurecr.io/gatekeeper:$(GATEKEEPER_VERSION) | |||
| GOTESTSUM = gotest.tools/[email protected] | |||
|
|
|||
| # Golang version go mod tidy compatibility | |||
| GOLANG_VERSION ?= 1.20 | |||
There was a problem hiding this comment.
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.
go mod tidy is expecting an x.y version, thus we can't do that.
go mod tidy -compat=1.20.12
invalid value "1.20.12" for flag -compat: expecting a Go version like "1.20"
usage: go mod tidy [-e] [-v] [-x] [-go=version] [-compat=version]
Run 'go help mod tidy' for details.
make: *** [Makefile:283: go-tidy] Error 2
There was a problem hiding this comment.
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.
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.
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.
|
/azp run ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Please rebase pull request. |
Run 'go mod' functions to search for tidy, vendor changes and verify it
Update makefile and one doc file docomentation with the change of go-verify over using vendor and tidy
|
/azp run ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -18,6 +18,9 @@ GATEKEEPER_VERSION = v3.10.0 | |||
| GATEKEEPER_IMAGE ?= ${RP_IMAGE_ACR}.azurecr.io/gatekeeper:$(GATEKEEPER_VERSION) | |||
| GOTESTSUM = gotest.tools/[email protected] | |||
|
|
|||
| # Golang version go mod tidy compatibility | |||
| GOLANG_VERSION ?= 1.20 | |||
There was a problem hiding this comment.
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.
| @@ -18,6 +18,9 @@ GATEKEEPER_VERSION = v3.10.0 | |||
| GATEKEEPER_IMAGE ?= ${RP_IMAGE_ACR}.azurecr.io/gatekeeper:$(GATEKEEPER_VERSION) | |||
| GOTESTSUM = gotest.tools/[email protected] | |||
|
|
|||
| # Golang version go mod tidy compatibility | |||
| GOLANG_VERSION ?= 1.20 | |||
There was a problem hiding this comment.
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.
| @@ -18,6 +18,9 @@ GATEKEEPER_VERSION = v3.10.0 | |||
| GATEKEEPER_IMAGE ?= ${RP_IMAGE_ACR}.azurecr.io/gatekeeper:$(GATEKEEPER_VERSION) | |||
| GOTESTSUM = gotest.tools/[email protected] | |||
|
|
|||
| # Golang version go mod tidy compatibility | |||
| GOLANG_VERSION ?= 1.20 | |||
There was a problem hiding this comment.
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.
Which issue this PR addresses:
None. It is a small enhancement. Duplicate of #3492.
What this PR does / why we need it:
Run 'go mod' functions to search for tidy, and vendor package changes and then verify it.
Test plan for issue:
Is there any documentation that needs to be updated for this PR?
Yes, in the second commit
How do you know this will function as expected in production?