Skip to content
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

Jump to bottom

add support for go workspaces #445

Closed
wants to merge 1 commit into from
Closed

add support for go workspaces #445

wants to merge 1 commit into from

Conversation

aryan9600
Copy link
Member

Add a go.work file listing all modules in this monorepo and remove local replace directives from all go.mod files

Fixes #400

Signed-off-by: Sanskar Jaiswal [email protected]

@darkowlzz
Copy link
Contributor

darkowlzz commented Jan 9, 2023 •
edited
Loading

Thanks for adding workspaces. It helps the IDEs and gopls work nicely with this repository with so many modules. Would be good to have it in all the controllers and their API packages.

While research about it, I came across the issue of go build using the module dependency from workspace instead of local go.mod file, which may have unintended results. So the workspaces is turned off in the release environment, for example https://github.com/kubernetes-sigs/kustomize/blob/kustomize/v4.5.7/releasing/run-goreleaser.sh#L124 . I think we need something similar.
But at the same time, we don't build and publish binaries from this repository. So maybe not fully applicable.

@aryan9600 aryan9600 force-pushed the go-work branch 2 times, most recently from e996efb to 0b80e5e Compare February 5, 2023 12:26
darkowlzz
darkowlzz approved these changes Feb 6, 2023
View reviewed changes
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

Other than the gogit version set in git e2e test, this looks good to me.
But please have one/few more reviews on it before merging because it affects the whole repository.

git/internal/e2e/go.mod Outdated Show resolved Hide resolved
@aryan9600
add support for go workspaces
0c8c487 
Add a go.work file listing all modules in this monorepo and remove local
replace directives from all go.mod files

Signed-off-by: Sanskar Jaiswal <[email protected]>
stefanprodan
stefanprodan approved these changes Feb 6, 2023
View reviewed changes
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

We need to enable workspaces on all controllers, this way the CVE scanners should no longer be confused by which API version we're using. Right now scanners can't determine the version of a locally replaced package and they report old CVEs, but with workspaces the API version gets set in go.sum. The main difference to the release procedure, is that tagging the API is a must before opening the controller release PR.

@hiddeco
Copy link
Member

hiddeco commented Feb 6, 2023

Think the problem with the go.work file in the root of this repository is that now all modules are assumed to have a relationship. This isn't necessarily true, and seems to break the isolation of packages which didn't have any replacement directives in their go.mod files before.

@stefanprodan
Copy link
Member

I've done some testing with this, and having a single workspace makes all packages share the same dep , which is bad... also this PR contains un-synced deps, @aryan9600 have you've run go work sync?

@hiddeco hiddeco added the area/ci CI related issues and pull requests label Feb 6, 2023
@hiddeco
Copy link
Member

hiddeco commented Jun 22, 2023

Closing this as it does not seem a viable solution for now (and it has been stale for a long time).

@hiddeco hiddeco closed this Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanprodan stefanprodan stefanprodan approved these changes

@darkowlzz darkowlzz darkowlzz approved these changes

Assignees
No one assigned
Labels
area/ci CI related issues and pull requests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use go workspaces
4 participants
@aryan9600 @darkowlzz @hiddeco @stefanprodan