Skip to content

Add go.work to deal with multi module dependencies #79

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

Merged
openshift-merge-robot merged 1 commit into from
Nov 30, 2022
Merged

Add go.work to deal with multi module dependencies #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,7 @@ config/manager/kustomization.yaml

# Common CI tools repository
CI_TOOLS_REPO

# generated workspace file
go.work
go.work.sum
3 changes: 3 additions & 0 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ ARG OPERATOR_BASE_IMAGE=gcr.io/distroless/static:nonroot
# Build the manager binary
FROM $GOLANG_BUILDER AS builder

ARG GOWORK=off
ENV GOWORK=$GOWORK

#Arguments required by OSBS build system
ARG CACHITO_ENV_FILE=/remote-source/cachito.env

Expand Down
22 changes: 19 additions & 3 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ BUNDLE_GEN_FLAGS ?= -q --overwrite --version $(VERSION) $(BUNDLE_METADATA_OPTS)

VERIFY_TLS ?= true

# GOWORK
GOWORK ?= off
export GOWORK := $(GOWORK)

Copy link
Contributor

@Akrog Akrog Nov 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why don't we use the Make export construct so that it is present in all the executions?

That would save us from adding GOWORK everywhere in the Makefile

export GOWORK := $(GOWORK)

Copy link
Contributor Author

@fmount fmount Nov 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why don't we use the Make export construct so that it is present in all the executions?

That would save us from adding GOWORK everywhere in the Makefile

export GOWORK := $(GOWORK)

Not sure I'm properly following you here, but as per [1] (if it lands) I can remove all the GOWORK=$(GOWORK) occurrences in the Makefile and just have GOWORK ?= off at the top of the file as you previously suggested.

Then, if you run something like:

GOWORK=on make govet

you end up seeing something like [2], but if you run:

GOWORK=off make govet

(or just make govet) you'll see something like [3].

IMO this would simplify a lot the Makefile and we don't have to deal w/ that variable unless we would like to set it to on.

@Akrog does this make sense?

[1] openstack-k8s-operators/openstack-k8s-operators-ci#56
[2] https://paste.opendev.org/show/bLGg1Dx7D4ueNmqIT5kQ/
[3] https://paste.opendev.org/show/b48LCNwZFoOk3DdPKraI/

Copy link
Contributor

@Akrog Akrog Nov 29, 2022

Choose a reason for hiding this comment

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

If you just add:

GOWORK ?= off
export GOWORK := $(GOWORK)

Then you don't need to add GOWORK=$(GOWORK) anywhere in this file.
And then we you want to use the go workspace you can do:

GOWORK= make build

And it will use the current go workspace.

Copy link
Contributor Author

@fmount fmount Nov 30, 2022

Choose a reason for hiding this comment

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

My only point here is that we can avoid export GOWORK := $(GOWORK) if [1] lands.
However, there isn't much change in how govet.sh and the other scripts are called, I guess the only thing we should take care is documenting you have to GOWORK= make build (before this change you didn't have to).
If we combine this change with [1] we can't just avoid the export directive (which defaults to off because of the CI execution) and keep the user experience as it is (and in line with the other operators).

[1] openstack-k8s-operators/openstack-k8s-operators-ci#56

Copy link
Contributor

@Akrog Akrog Nov 30, 2022

Choose a reason for hiding this comment

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

I think everything in Makefile should not use the go workspace by default, That way build and run also ignores it unless explicitly requested with the GOWORK=.

I'm thinking about leftover go.work replaces after testing a PR and then forgetting about it and doing make build and getting all kinds of issues.

I believe it is less likely to forget adding the GOWORK= when we are working with dependencies than the other way around.

The documentation PR already includes the GOWORK= when using external dependencies.

Copy link
Contributor Author

@fmount fmount Nov 30, 2022

Choose a reason for hiding this comment

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

I think everything in Makefile should not use the go workspace by default, That way build and run also ignores it unless explicitly requested with the GOWORK=.

I'm thinking about leftover go.work replaces after testing a PR and then forgetting about it and doing make build and getting all kinds of issues.

I believe it is less likely to forget adding the GOWORK= when we are working with dependencies than the other way around.

Agreed, tbh I didn't have a strong opinion but as you mentioned, it's good making sure that people understand how to explicitly turn it on instead of messing with potential go.work changes (that would end up "working on the laptop").
Being that said, I added the export directive in the last iteration.

The documentation PR already includes the GOWORK= when using external dependencies.

Thank you, sounds good!

# USE_IMAGE_DIGESTS defines if images are resolved via tags or digests
# You can enable this value if you would like to use SHA Based Digests
# To enable set flag to true
Expand Down Expand Up @@ -117,11 +121,17 @@ fmt: ## Run go fmt against code.

.PHONY: vet
vet: ## Run go vet against code.
go vet ./...
go vet ./... ./api/...

.PHONY: test
test: manifests generate fmt vet envtest ## Run tests.
KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" go test ./... -coverprofile cover.out
KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" go test ./... ./api/... -coverprofile cover.out

.PHONY: gowork
gowork: ## Generate go.work file to support our multi module repository
Copy link
Contributor

@Akrog Akrog Nov 30, 2022

Choose a reason for hiding this comment

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

Do we still want this even though we don't use it anymore? (just checking, I don't have a strong opinion)

Copy link
Contributor Author

@fmount fmount Nov 30, 2022

Choose a reason for hiding this comment

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

Given it's an isolated target and we don't call it explicitly, for development purposes it's good to have it here in case you're fighting with the go dependencies model. I added it under "Development", so you can see it in the right column (see make help)

test -f go.work || go work init
go work use .
go work use ./api

##@ Build

Expand All @@ -135,7 +145,7 @@ run: manifests generate fmt vet ## Run a controller from your host.

.PHONY: docker-build
docker-build: test ## Build docker image with the manager.
podman build -t ${IMG} .
podman build --build-arg GOWORK=$(GOWORK) -t ${IMG} .

.PHONY: docker-push
docker-push: ## Push docker image with the manager.
Expand Down Expand Up @@ -273,19 +283,25 @@ get-ci-tools:
# Run go fmt against code
gofmt: get-ci-tools
$(CI_TOOLS_REPO_DIR)/test-runner/gofmt.sh
$(CI_TOOLS_REPO_DIR)/test-runner/gofmt.sh ./api

# Run go vet against code
govet: get-ci-tools
$(CI_TOOLS_REPO_DIR)/test-runner/govet.sh
$(CI_TOOLS_REPO_DIR)/test-runner/govet.sh ./api

# Run go test against code
gotest: get-ci-tools
$(CI_TOOLS_REPO_DIR)/test-runner/gotest.sh
$(CI_TOOLS_REPO_DIR)/test-runner/gotest.sh ./api

# Run golangci-lint test against code
golangci: get-ci-tools
$(CI_TOOLS_REPO_DIR)/test-runner/golangci.sh
$(CI_TOOLS_REPO_DIR)/test-runner/golangci.sh ./api

# Run go lint against code
golint: get-ci-tools
PATH=$(GOBIN):$(PATH); $(CI_TOOLS_REPO_DIR)/test-runner/golint.sh
PATH=$(GOBIN):$(PATH); $(CI_TOOLS_REPO_DIR)/test-runner/golint.sh ./api

6 changes: 6 additions & 0 deletions go.work
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
go 1.18

use (
.
./api
)