🌱 Add go.work to .gitignore and .dockerignore

[Jont828]

What this PR does / why we need it: VSCode doesn't support nested modules like test/infrastructure/docker and will not give autocomplete or linting suggestions. Adding a go.work file allows VSCode to recognize the nested modules.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

[sbueringer]

I wonder how folks were developing with vscode in the past. Maybe there's a way to configure vscode?

In intellij everything just works and I'm hesitant to use go workspaces just for vscode. (I'm not sure what the implications for go.mod/go.sum are if we start using go workspaces)

[chrischdi]

I wonder how folks were developing with vscode in the past. Maybe there's a way to configure vscode?

In intellij everything just works and I'm hesitant to use go workspaces just for vscode. (I'm not sure what the implications for go.mod/go.sum are if we start using go workspaces)

I mostly add a go.work file but do not check it in into git ... (comes with the cons of maybe not all make targets will work anymore)

[Jont828]

Sure, I'd be happy to hear what others have been doing to get VSCode working. I figured that if everyone was using this same solution, it'd make sense to make it a common file instead of having an additional step to configure it every time you clone the repo.

[sbueringer]

Not sure, but looks like it breaks our e2e test

[richardcase]

Interestingly, i see a few places saying that the go.work shouldn't be committed to git. Its also in a few template gitignore files, for example: https://github.com/github/gitignore/blob/main/Go.gitignore

[sbueringer]

Let's see how other folks are using it.

Might be an alternative to:

• document that folks should add this file
• add it to .gitignore
• probably (?) add it to dockerignore to fix our tests / image builds when the file exists
  • Tested it locally as well as soon as go.work exists things like make docker-build-e2e fail (adding it to .dockerignore fixes that)

[sbueringer]

cc @vincepri @ykakarap @fabriziopandini How are you dealing with this when you're using VSCode for CAPI?

[Jont828]

Hmm if we don't want to commit the file maybe we can add a Makefile target that inits the go.work file and sets it up. Will adding it to Dockerignore be enough to make it not break the make docker-build-e2e target?

[sbueringer]

Will adding it to Dockerignore be enough to make it not break the make docker-build-e2e target?

Worked on my machine

[Jont828]

@sbueringer I made some changes to add the make targets, lmk how that works for you!

[Jont828]

/retest

[killianmuldoon]

IMO it's a good idea to add it to the ignore files but I don't think the make targets are useful unless there's clear guidance on how to use workspaces for CAPI dev.

Could we just include the .gitignore and .dockerignore until we have some best practices?

[richardcase]

I like the idea of the make target, this is helpful 👍 And with the entries in the ignore files.

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 656c36930ba51f2fff7f915b4bfdf00c83e38a38

[killianmuldoon]

I'm not sure what problem putting go.work files at the top-level in the CAPI repo is solving, so I wouldn't include them in the Makefile without a clear picture of that.

Inspired by this (thanks @Jont828!) I've been playing around with workspaces to solve #6081. I think what we'd want in CAPI is workspaces where we currently have modules with replace directives i.e. in the hack/tools and test folders. There's a WIP PR is here: https://github.com/kubernetes-sigs/cluster-api/pull/8208/files

[chrischdi]

I'm not sure what problem putting go.work files at the top-level in the CAPI repo is solving, so I wouldn't include them in the Makefile without a clear picture of that.

Inspired by this (thanks @Jont828!) I've been playing around with workspaces to solve #6081. I think what we'd want in CAPI is workspaces where we currently have modules with replace directives i.e. in the hack/tools and test folders. There's a WIP PR is here: https://github.com/kubernetes-sigs/cluster-api/pull/8208/files

I think having go.work files at the sub-package level does not make sense IMHO.

With Go workspaces, you control all your dependencies using a go.work file in the root of your workspace directory. The go.work file has use and replace directives that override the individual go.mod files, so there is no need to edit each go.mod file individually. [0]

What the top-level go.work file solves is:

The used go language server gopls (which is used in VSCode, and maybe also other IDE's) is then able to support the nested go modules, when opening the top-level go module at the IDE. By support I mean that I can open the top-level repository in the IDE and have the language server features working like resolving functions, variables, ... , what otherwise does not work.

[richardcase]

I'm not sure what problem putting go.work files at the top-level in the CAPI repo is solving, so I wouldn't include them in the Makefile without a clear picture of that.

@killianmuldoon - its solving the problem where vscode complains about editing a nested package:

[killianmuldoon]

Thanks for the explanation @chrischdi - was really helpful 🙂.

And thanks for the error message @richardcase. I think I'll let others have their say, but I'm fine with this with or without the make targets.

[sbueringer]

Can we follow the pattern introduced in #8088 instead of creating a make target?

We already have so many make targets and they are usually used for our daily work and not a for a "once-in-a-lifetime" operation like creating a go.work file for VSCode users.

[Jont828]

@sbueringer @killianmuldoon I think the main thing is that we need to generate the go.work file instead of it being like a template you can copy/paste. I figured we could add something so folks wouldn't have to figure which how to set up the go work use command since everyone would have to run the following lines. But I'm fine with just adding it to the .gitignore and .dockerignore as a starting point.

go work init
go work use . ./hack/tools ./test

[sbueringer]

Why does it have to be generated?

I'm probably wrong but it looked pretty static to me

[Jont828]

I'm not sure, we're just expected in the docs to init the file, probably because it has a go.work.sum file along with it.

[sbueringer]

Ah I see. Yeah we can't check in the go.work.sum file.

Independent of that I think it would be better if we would document dev/IDE setup in the book and mention there that folks should run those two commands. I don't think a lot of folks will just find this make target. And also as mentioned above it seems like this is only ever run once. So overall I don't think a Make target is a good way to document this for contributors.

Maybe we can merge this PR with only the ignore files and then create a follow-up issue to introduce a dedicated dev/IDE setup page in the book where we can mention that?

[fabriziopandini]

Sorry, for getting to this discussion late
Someone can help me in understanding why we can't have go.work at top level, it is an official go feature, and if it makes happy both VSCode and Intellij/golang users (and others if we have them in the radar), then why not?
We are at the beginning of 1.5.0, we can also consider using workspaces for some time and then eventually roll-back

[Jont828]

@fabriziopandini I think that would make sense as well since it's an official feature as you said. The only thing is that the some of the make targets for build and e2e tests don't work with the go.work file. I don't know if that's supposed to happen or if we'd need to make an additional change.

[killianmuldoon]

Someone can help me in understanding why we can't have go.work at top level, it is an official go feature, and if it makes happy both VSCode and Intellij/golang users (and others if we have them in the radar), then why not?

There's ongoing discussion at the go repo about this - golang/go#53502. My inclination is not to include it unless there's some clear benefit. I agree with @sbueringer that this is better in a doc than in a readme - now that we have a dev/vscode-example-configuration folder there's a good place to include a guide for individual IDEs.

It would be good to drop the make target from this PR and move broader discussion about CAPI using workspaces to an issue.

[Jont828]

@killianmuldoon @sbueringer I've went ahead and dropped this from the make targets. I think we can merge this so it'll work with the .gitignore and .dockerignore for now. WDYT?

[sbueringer]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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:

• OWNERS [sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 8cbe3d45e8c722d02ac058d9546aa6bc12814170

[johannesfrey]

/area devtools