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.

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

add support for go workspaces #445

Closed
wants to merge 1 commit into from
Closed

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

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.

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
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]>
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
Labels
area/ci CI related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use go workspaces
4 participants