crosslink: Support go.work

[pellared]

Why

Towards open-telemetry/opentelemetry-go#3964

go.work is honored by gopls language server so that develoeprs get better UX in tools like VSCode, Vim, Emacs etc

References:

• https://go.dev/ref/mod#workspaces
• https://go.dev/blog/get-familiar-with-workspaces
• https://go.dev/doc/tutorial/workspaces

What

Make crosslink to update the use in go.work file (if it exists).

Take notice that it will not harm any repositories which do not use Go workspaces. However developers could still use the functionality during development 😉

PS. The tests have a lot of repetition . However refactoring it in this PR would make it very hard to review. I can refactor all crosslink unit tests in next PR.

Testing

You can check the result on this branch and on open-telemetry/opentelemetry-go#4083

[codecov]

Codecov Report

Patch coverage: 74.28% and project coverage change: +0.63 🎉

Comparison is base (653d34f) 58.99% compared to head (3fe231b) 59.62%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #308      +/-   ##
==========================================
+ Coverage   58.99%   59.62%   +0.63%     
==========================================
  Files          46       46              
  Lines        2024     2120      +96     
==========================================
+ Hits         1194     1264      +70     
- Misses        698      716      +18     
- Partials      132      140       +8     

Impacted Files	Coverage Δ
crosslink/internal/crosslink.go	68.46% <64.51%> (-3.13%)	⬇️
crosslink/internal/prune.go	72.61% <72.97%> (-0.30%)	⬇️
crosslink/internal/common.go	74.57% <83.78%> (+15.48%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pellared]

It even is not clear how the go.work.sum is updated and how it is used so I see no reason to add it. Reference golang/go#51941

[bryan-aguilar]

Nice! This looks cool, I'm glad you have a use case for this and thanks for contributing it back!

My one question is, should updating a files go.work and go.mod files always happen at the same time. With the current implementation this seems to imply that is what a user would always want.

Is there a scenario where you would only want to update go.mod files and not go.work files? What about the other way around?

Should users be able to opt in or out of these updates through command flags? Should we instead be separating the updating of a go.work file behind a different cobra command?

[bryan-aguilar]

Also, should we support the use case of creating a go.work file if it does not exist? Why should having a preexisting go.work file be a prerequisite?

[bryan-aguilar]

Should we be checking this go.work file in? Is this intentional? From what I remember the go.work docs had suggested that these were not suitable for checking in, and only existed for local development. Have we identified a use case where that is not true?

[pellared]

From what I remember the go.work docs had suggested that these were not suitable for checking in, and only existed for local development.

From golang/go#53502:

For large monorepos with multiple modules, it likely makes sense to check in go.work, as it makes the setup easier and often has no downsides.

I think this is exactly the use case when crosslink is used.

The benefit of commiting it is that all developers can profit from it.

EDIT: Also golang/go#53502 (comment)

EDIT 2: I have no idea why GitHub says that this comment is outdated 😆

[pellared]

Anyway if you still have an opinion that it should be not committed I can remove it and add to .gitignore 😉

[pellared]

Removing go.work because of golang/go#60126.

[pellared]

3fe231b

[pellared]

Also, should we support the use case of creating a go.work file if it does not exist? Why should having a preexisting go.work file be a prerequisite?

I was thinking about it as well. For sure it should be an opt-in functionality as somebody may want NOT to use Go workspaces. I found following reasons why I did not want to add such functionality (at least in this PR):

1. Adding a new go.work file is simply done by running go work init. I do not think it is worth to make crosslink more complex is creating the file is so easy.
2. We would probably need to add a logic for "guessing" a proper Go version. For instance in this repo I had to manually change go 1.20 to go 1.19
3. YAGNI 😉 This can be always added later

[pellared]

Is there a scenario where you would only want to update go.mod files and not go.work files? What about the other way around?

I was thinking about it as well and I have not come up with such scenario. I do not see why anyone would like to update only go.mod without updating go.work and vice-versa. Doing it separately may make the repo "out-of-sync".

I see a use case. Someone may only use go.work and not care about having replace in go.mod files 😉 Do you have any proposal how the CLI should look like? Personally, I propose to add flags to ignore processing of go.mod or/and go.work files. Then by default user can still update both type of files using one command. Also prune could still support both type of files.

Personally, I propose to add it as a separate PR to make it easier to review. Also I do not think it is a blocker as this can be always added on top of existing PR.

[pellared]

@bryan-aguilar After longer thoughts I am considering to change the code to have a new gowork command. Then the command could generate a go.work file if it does not exist. What do you think?

[bryan-aguilar]

@bryan-aguilar After longer thoughts I am considering to change the code to have a new gowork command. Then the command could generate a go.work file if it does not exist. What do you think?

I think this makes sense. My suggestion would have been very similar. I would expect the cli interface to be something like crosslink gowork. I am sure there will be some flags that are shared between the root and gowork command.

[pellared]

Converting to draft. I will change this PR to introduce a work command. ETA Monday/Tuesday.

[pellared]

Closing this PR in favor of #311

CC: @bryan-aguilar @hanyuancheung