-
Notifications
You must be signed in to change notification settings - Fork 17.4k
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.
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
proposal: ref/mod: mention whether go.work files should be checked into VCS #53502
Comments
@bcmills points out that perhaps there isn't consensus about whether to check We could recommend never checking them in, but I think that's a bad idea: in some particular scenarios it can make perfect sense to check them in, such as monorepos holding multiple modules. We could continue with no recommendation at all, but I worry that then we'll continue to see third party docs and sites make assertive recommendations like "never check them in". I personally think that we should give some general guidance that isn't too strict one way or another, such as:
This needs a decision that isn't easy to make, and we should probably get input from others, so @bcmills suggests turning this issue into a proposal. Doing that. |
I've noticed that gopls isn't able to "Find references" for an identifier in a "nested" module unless I create a I couldn't get VSCode or Neovim to make it work in any other way - I just had to create a I would hate to have to teach people in my team how to create a @mvdan I'm not sure if my example project is what you were referring to as "monorepos", but wanted to give it as an example Maybe it's just a gopls bug and so this can't be used as an argument in favor of checking in |
Assuming your nested Go modules all live in the same git repository as the main module, then yes, that's the sort of setup that I mean by a monorepo. A VCS monorepo will typically contain dozens of modules, and in general nested modules might be tricky and could be avoided, but the end result is still the same: it is possible and perhaps reasonable to check in a |
The If you put it inside your Go module (monorepo or not) and add My current working theory about my own use of |
FWIW, you should be able to set |
Those environment variables does not automatically propagate across all the environments in an open source project. I have said this before, but may as well repeat it, I think having this enabled by default was the wrong decision. We had a situation where you had to make sure that any replacements in |
I've come across quite a few situations where I've written a library and made use of a go.work file to provide a tangible benefit: I can easily have sub-modules, run Two use-cases I had recently: I wanted to write some tests for a package that works with errors. These tests want to ensure my library works nicely with all the other errors packages. So this means I'm importing 10 different errors packages into that The other case was I wrote a package that integrates with various other packages. Of course each binding layer is a separate module so, again, the root level I thought this was fine until someone on the Gopher slack told me this was bad practice, I couldn't really find any definitive answer and was linked to here. All of what I did was based on official documentation, not blog posts or anything else. So it wasn't really clearly communicated that what I was doing (despite making my workflow easier) was actually not the intended use of So it seems I've stumbled on some use cases that make my workflow easier, but are considered bad practice. This feels very un-Golang. I'd rather the tooling allow me to do things that are good practice, not not allow them at all, so there's no room for opinions and discussion (like the K&R thing - no arguments, just one style.) Thanks for raising this though! I'm keen to see a definitive outcome, and I'm not strongly opinionated one way or the other. 😊 |
@Southclaws for both of your scenarios, when a user imports your package and then creates their own So I don't think either scenario you describe really makes sense for a |
Ah thanks, I didn't know this! I figured the lack of explicit dependencies/devDependencies (like npm) meant this wasn't the case. Given that, I see no reason to not ignore go.work files, it would be worth mentioning this in the official docs for sure! |
for more info https://dev.to/gophers/what-are-go-workspaces-and-how-do-i-use-them-1643 golang/go#53502 (comment) Co-authored-by: HuangYi <huang@crypto.com>
I think we should strongly caution against checking in go.work files, but there are some rare cases where it makes sense to check them in so we have to leave some room for that possibility. Here's proposed wording to add to ref/mod. What do y'all think:
|
This would be great, thanks @matloob I'd maybe organize the text a bit differently (*), but the overall idea is solid. (*) Start by saying it's not recommended and list the disadvantages as a bullet list. |
Compared to my ealier attempt at #53502 (comment), I like that it explains a couple of pitfalls with checked-in go.work files. I agree with @eliben that I would like to see better formatting, perhaps splitting into 1-2 paragraphs with a bullet list. However, I'm not a big fan of the wording "rare cases"; I think monorepos (or multi-module repos) are relatively common, particularly in work scenarios. I worry that only listing disadvantages, and saying that there "may be rare cases where it might make sense", is going to give the idea that it's heavily frowned upon. When, in practice, I think an enterprise monorepo with many modules should definitely check in a go.work file. They just need to be mindful of the pitfalls. |
How about the following? I softened up the rare cases wording because I don't have visibility into how modules are structured in work scenarios but I want to be really careful about not recommending In general, it is recommended that go.work files not be checked into version control systems. Module developers should think carefully before checking in
With that said there are some cases where it might make sense to do so: particularly if the workspace modules don't have any other modules that depend on them. |
The gosec tool can only scan multi module projects with a Depending on the project, think of the following when deciding to add the
About a monorepo, does it generally make sense to commit the |
@Emptyless I'm very surprised looking at the comments on your review. We should certainly not force people who work in multi module repositories to check in their go.work files and should support users working without checked-in go.work files. That's how you get the best experience with go.work files. go.work files are meant to be a convenience feature to make certain workflows easier- not a requirement. I tried to hit the tone in my proposed wording that in some cases it is okay to check in go.work files but I certainly don't want to encourage doing so or even worse require it. The best experience for a monorepo is to have a single module. Because there are cases where that's not possible, for example because different components of a monorepo are distributed and versioned separately, but that's exactly the decoupling that you mentioned. |
I would think exactly the opposite. In many (most? nearly every?) cases a monorepo will contain multiple things and those things would typically be versioned independently (pretty much the definition of a Go module). Having a monorepo be a single Go module feels like the opposite of what I would want/expect. I use VS Code and it does in fact require that you have a At work we've settled on option 2 for repos with >1 module, which are becoming more common for us. |
go.work is not respected by imports, which means that our test environment - if it uses go.work - will differ from what'll be used by third parties which import our core module. This commit adds a generation target for go.work which will allow users to opt-in to using it locally without it being enabled by default for everyone. See golang/go#53502 for discussion on whether or not go.work should be checked in. Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
@matloob did you get any significant objections to this? If not, can this be "implemented" (by mentioning in the right docs)? |
I haven't gotten any significant objections. I'll send a CL to add this to ref/mod shortly. |
Change https://go.dev/cl/580115 mentions this issue: |
Would it be useful, because there is a tutorial on how to use workspaces and the workflow that it represents, to provide a tutorial/workflow where checking the go.work would be beneficial? The use case that say |
These files should not be checked into version control as per community convention (see: golang/go#53502). Additionally, they are incompatible with renovatebot. Signed-off-by: Michael Adler <michael.adler@siemens.com>
These files should not be checked into version control as per community convention (see: golang/go#53502). Additionally, they are incompatible with renovatebot. Signed-off-by: Michael Adler <michael.adler@siemens.com>
These files should not be checked into version control as per community convention (see: golang/go#53502). Additionally, they are incompatible with renovatebot. Signed-off-by: Michael Adler <michael.adler@siemens.com>
Should make it easier for a new developer to get their environment up and running. You could also use a relative path, i.e. `go work use -r .`, but here we choose to use the absolute path since that's how @rnester-rh does it. See golang/go#53502 for some background on why we don't check in the workspace file.
These files should not be checked into version control as per community convention (see: golang/go#53502). Additionally, they are incompatible with renovatebot. Signed-off-by: Michael Adler <michael.adler@siemens.com>
https://go.dev/ref/mod#workspaces describes the current implementation of Go workspaces, as proposed by https://golang.org/issue/45713, which culminated in the proposal doc at https://go.googlesource.com/proposal/+/master/design/45713-workspace.md.
The proposal doc says:
However, the canonical documentation about
go.work
at https://go.dev/ref/mod#workspaces makes no mention of this, as far as I can see. In practice, it seems like the common knowledge among Go developers is that the file should never be checked in, because:gitignore
includes the file since Go: Ignore Go workspace file go.work github/gitignore#3884x/tools
does not include one, even though it has two modules connected by a directory replace directiveFirst, we should decide whether we actually want to recommend users to never check in
go.work
files, like the original proposal doc said. If that's the case, then we simply need to add that recommendation to the/ref/mod
page.However, if we don't want to make that general recommendation, then we need to figure out under what circumstances it is recommended or not to check in
go.work
into VCS. For example, here are some personal guesses of mine:go.work
is likely a bad idea, as it prevents downstream users from easily setting up their owngo.work
in a parent directory.go.work
, as it makes the setup easier and often has no downsides.go.work
files will naturally live in the parent directory to the VCS clones, so they have no VCS repository to naturally be checked into at all.cc @bcmills @marwan-at-work @matloob
The text was updated successfully, but these errors were encountered: