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 new makefile targets for go mod verification #3550

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add new makefile targets for go mod verification #3550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e002c6e
Add new makefile targets for go mod verification
razo7 May 22, 2024
43f6765
Update documentation on go-verify changes
razo7 May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions .github/workflows/ci-go.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ jobs:
- name: Checkout repository
uses: actions/checkout@v4
- run: |
go mod vendor
go mod tidy -compat=1.20
make go-verify
hack/ci-utils/isClean.sh

generate-check:
Expand Down
16 changes: 16 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ GATEKEEPER_VERSION = v3.10.0
GATEKEEPER_IMAGE ?= ${RP_IMAGE_ACR}.azurecr.io/gatekeeper:$(GATEKEEPER_VERSION)
GOTESTSUM = gotest.tools/gotestsum@v1.11.0

# Golang version go mod tidy compatibility
GOLANG_VERSION ?= 1.20
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

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


ifneq ($(shell uname -s),Darwin)
export CGO_CFLAGS=-Dgpgme_off_t=off_t
endif
Expand Down Expand Up @@ -275,9 +278,22 @@ admin.kubeconfig:
aks.kubeconfig:
hack/get-admin-aks-kubeconfig.sh

.PHONY: go-tidy
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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

vendor:
# See comments in the script for background on why we need it
hack/update-go-module-dependencies.sh
$(MAKE) go-verify

install-go-tools:
go install ${GOTESTSUM}
Expand Down
10 changes: 4 additions & 6 deletions docs/updating-dependencies.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ The reason for calling script instead of directly calling:

```bash
go get -u ./...
go mod tidy -compat=1.20
go mod vendor
make go-verify
```

is that packages modified in this script do not fully support modules and
Expand All @@ -43,17 +42,16 @@ the PR, one can simply call
go get <module>@<release> OR
go get -u <module>@<release>

go mod tidy -compat=1.20
go mod vendor
make go-verify
```

---

**NOTE** vendoring is required as ARO mirrors all dependencies locally for the CI reliability
and reproducibility.

**NOTE** that when running `go mod vendor` only modules that are used in the
source code will be vendored in.
**NOTE** that running `make go-verify` adds missing (and remove unused) modules,
make a vendored copy to the *vendor* direcory and then verifies that the dependencies have expected content.

**NOTE** when updating a package modified in `hack/update-go-module-dependencies.sh`
changes have to be made there also. Otherwise next run of `make vendor` will
Expand Down
2 changes: 0 additions & 2 deletions hack/update-go-module-dependencies.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,3 @@ done

go get -u ./...

go mod tidy -compat=1.20
go mod vendor