Add go.work to deal with multi module dependencies #79
Conversation
|
@Akrog with this change to the and run |
Makefile
Outdated
| gowork: ## Generate go.work file to support our multi module repository | ||
| test -f go.work || go work init | ||
| go work use . | ||
| go work use ./api |
There was a problem hiding this comment.
-1: I would add the go.work directly in the repo instead of having this line
There was a problem hiding this comment.
So you're suggesting running those 3 commands and put the resulting files (go.work and go.work.sum) within the repository: am I understanding correctly?
By doing that we can avoid this target here.
There was a problem hiding this comment.
FYI, we added this target to other operator Makefile as the design workspace doc says https://go.googlesource.com/proposal/+/master/design/45713-workspace.md#multiple-modules-in-the-same-repository-that-depend-on-each-other
These go.work files should not be checked into the repositories so that they don‘t override the workspaces users explicitly define. Checking in go.work files could also lead to CI/CD systems not testing the actual set of version requirements on a module and that version requirements among the repository’s modules are properly incremented to use changes in the modules.
Why we could do different, this was the initial reason why we did not.
There was a problem hiding this comment.
Instead of
go mod edit -replacelike we suggest I think it's better to dogo work edit -replacebecause that way our local changes will be ignored by git since go.work is in .gitignore That's what I'm going to suggest in the docs.
When a commit depends on another commit in another repository I tend to actually check in and publish the replace in go.mod so that when people review the PR they can pull it and run it locally with the correct deps. Sure this means once the dependency merges I need to respin my PR but overall but this is my low tech version of zuul's Depends-On:
There was a problem hiding this comment.
+1 and thanks @gibizer for sharing. The whole point of having go.work (in my recent experience) is to make the go mod edit -replace working properly when a local lib-common is referenced in the project.
I didn't find a nice way to make it working without creating a go workspace.
It would be great (in the next future) having our CI able to checkout the dependencies and run a job which takes into account the PR dependencies.
There was a problem hiding this comment.
I like to have it explicitly stated in the commit message (like with th eDepends-on you mention).
There was a problem hiding this comment.
@stuggi Iiuc that paragraph is referring to not checking in the
go.workwith actual replacements, which is also what I'm suggesting.By having the
go.workwith only the.and./apiin the repo and excluding it with.gitignorewe avoid anyone from inadvertently submitting it.
right, we definitely want to have it in .gitignore to prevent people to add it to commits. Having a default checked in is something we could do, but its not common practice. So far I have not found other projects which do this. In our case for the main and api sub module its probably ok to add the go.work to the repo as they depend on each other.
There was a problem hiding this comment.
Ok, I see @stuggi's point of view and in general I agree about not having go.work and go.work.sum in the repos.
However, I understand that PR #77 mentioned by @Akrog introduces a series of scripts with the goal of simplify the developer life, automating the way we checkout PRs [1] and set dependencies to GOWORK [2] in cinder-operator.
In this case I suspect it's good to maintain consistency with the direction taken by this operator, and we can actually commit a go.work file (which is still in .gitignore), removing the need to execute the gowork target each time.
We might want to keep the gowork target in the Makefile (it can be explicitly called if needed), but I can remove its execution in the vet target (and wherever it occurs).
I'm looking for quorum to move forward with this, so I'd like to have your feedback @stuggi @Akrog
[1] https://github.com/openstack-k8s-operators/cinder-operator/pull/77/files#diff-adc3e455779ae5d167129a8b2a045ac46868c35ea47fcfb127327f0c3ba7cf45
[2] https://github.com/openstack-k8s-operators/cinder-operator/pull/77/files#diff-58fb17599cdc1f9bd44fbdb460b74f3de822da6a4f5007f0ac97831ee0e4ec68
There was a problem hiding this comment.
I am fine with the proposal, adding the default go.work with having it in .gitignore and remove calling it in the targets.
There was a problem hiding this comment.
If we really don't want to have the go.work file included to be the same as other projects I could modify my scripts to create it when necessary, though we would still want to default GOWORK to off
Ah nice, I didn't know we couldn't have a |
fmount
force-pushed
the
gowork
branch
2 times, most recently
from
a15698c to
ec4b175
Compare
The cinder-operator patch [1] introduces the 'go.work' file within the repository with the goal of automating the way we deal with multi module dependencies. However, the side effect of having a versioned 'go.work' in the repo is the failure of this script because GOWORK is automatically set to "on". This patch fixes the CI run on the cinder-operator PR [1], introducing a GOWORK variable (which is set to off by default) to make sure we can successfully run this check regardless of the Makefile approach. [1] openstack-k8s-operators/cinder-operator#79 Signed-off-by: Francesco Pantano <[email protected]>
|
The patch [1] should fix the failure we've seen in CI as a side effect of having a versioned |
you can add |
The cinder-operator patch [1] introduces the 'go.work' file within the repository with the goal of automating the way we deal with multi module dependencies. However, the side effect of having a versioned 'go.work' in the repo is the failure of this script because GOWORK is automatically set to "on". This patch fixes the CI run on the cinder-operator PR [1], introducing a GOWORK variable (which is set to off by default) to make sure we can successfully run this check regardless of the Makefile approach. [1] openstack-k8s-operators/cinder-operator#79 Signed-off-by: Francesco Pantano <[email protected]>
| @@ -40,6 +40,9 @@ BUNDLE_GEN_FLAGS ?= -q --overwrite --version $(VERSION) $(BUNDLE_METADATA_OPTS) | |||
|
|
|||
| VERIFY_TLS ?= true | |||
|
|
|||
| # GOWORK | |||
| GOWORK ?= off | |||
|
|
|||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Why don't we use the Make
exportconstruct so that it is present in all the executions?That would save us from adding GOWORK everywhere in the
Makefileexport 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/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think everything in
Makefileshould not use the go workspace by default, That way build and run also ignores it unless explicitly requested with theGOWORK=.I'm thinking about leftover
go.workreplaces after testing a PR and then forgetting about it and doingmake buildand 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!
Yes, we might need something like: within the Dockerfile, and we might need to update the other operators as well. |
Yes, that or adding after So it's global to all the go commands we could run under the builder. |
| 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 |
There was a problem hiding this comment.
Do we still want this even though we don't use it anymore? (just checking, I don't have a strong opinion)
There was a problem hiding this comment.
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)
This patch aligns the cinder-operator to the work already done in both glance and openstack operator where go.work is added to deal with multi module dependencies and run linters on API module. A go.work default file has been added to the repository, and the related target is created in the Makefile. However, both vet and generate targets don't call gowork, and we rely on the default go.work file. Signed-off-by: Francesco Pantano <[email protected]>
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Akrog, fmount, stuggi 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:
Approvers can indicate their approval by writing |
…ators/renovate/github.com-openstack-k8s-operators-glance-operator-api-digest Update github.com/openstack-k8s-operators/glance-operator/api digest to 7730a6b
This patch aligns the cinder-operator to the work already done in both glance and openstack operators where go.work is added to deal with multi module dependencies and run linters on API module.
Signed-off-by: Francesco Pantano [email protected]