Skip to content

internal/util: don't panic if project directory is outside go path #1417

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

Closed

Conversation

gflarity
Copy link

@gflarity gflarity commented May 11, 2019

MustSetGopath was panic'ing (log.Fatalf) if the project directory wasn't in $GOPATH. However, with go modules this is perfectly reasonable now. Since go modules are default, I figured this should just be removed. Perhaps a new check should be added when under '--dep-manager dep'? I'm open to adding that with some direction, but this unblocked me for now.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 11, 2019
@openshift-ci-robot
Copy link

Hi @gflarity. Thanks for your PR.

I'm waiting for a operator-framework or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 11, 2019
@joelanford
Copy link
Member

@gflarity Thanks for the PR! This something that's on our list to do.

The plan is to remove the requirement for projects that are using go modules to be in $GOPATH, but it requires more than just this change:

  • For projects that are still using dep, which we plan to support at least until Go 1.13 is released, the requirement for $GOPATH needs to remain.
  • Some of the other CLI commands require projects to be in $GOPATH so that the project repo can be deduced.

Repo: projutil.CheckAndGetProjectGoPkg(),

Repo: projutil.CheckAndGetProjectGoPkg(),

We do this in a bunch of places:

$ grep -rn 'projutil.CheckAndGetProjectGoPkg()' cmd internal pkg version
cmd/operator-sdk/internal/genutil/openapi.go:36:	repoPkg := projutil.CheckAndGetProjectGoPkg()
cmd/operator-sdk/internal/genutil/k8s.go:35:	repoPkg := projutil.CheckAndGetProjectGoPkg()
cmd/operator-sdk/migrate/cmd.go:58:	_ = projutil.CheckAndGetProjectGoPkg()
cmd/operator-sdk/migrate/cmd.go:75:		Repo:           projutil.CheckAndGetProjectGoPkg(),
cmd/operator-sdk/migrate/cmd.go:129:		Repo:           projutil.CheckAndGetProjectGoPkg(),
cmd/operator-sdk/add/controller.go:81:		Repo:           projutil.CheckAndGetProjectGoPkg(),
cmd/operator-sdk/add/api.go:102:		Repo:           projutil.CheckAndGetProjectGoPkg(),
cmd/operator-sdk/up/local.go:186:		PackagePath: filepath.Join(projutil.CheckAndGetProjectGoPkg(), scaffold.ManagerDir),
cmd/operator-sdk/new/cmd.go:151:		Repo:           filepath.Join(projutil.CheckAndGetProjectGoPkg(), projectName),
cmd/operator-sdk/build/cmd.go:190:			PackagePath: filepath.Join(projutil.CheckAndGetProjectGoPkg(), scaffold.ManagerDir),
cmd/operator-sdk/build/cmd.go:244:				Repo:           projutil.CheckAndGetProjectGoPkg(),
cmd/operator-sdk/olmcatalog/gen-csv.go:80:		cfg.Repo = projutil.CheckAndGetProjectGoPkg()
internal/pkg/scaffold/internal/deps/print_go_mod.go:31:	repo := projutil.CheckAndGetProjectGoPkg()

Thinking off the top of my head here, we'll need to update projutil.CheckAndGetProjectGoPkg() to be able to return the repo path using another criteria if the project is using go modules (maybe by pulling the module path out of the go.mod file). And I think we'll also have to have the user to pass in the module path when creating a new project if it isn't in $GOPATH.

Thoughts?
/cc @estroz @lilic @shawn-hurley @theishshah @hasbro17 @AlexNPavel

@estroz
Copy link
Member

estroz commented May 15, 2019

Unfortunately using the module path in a go.mod file could cause issues if the project isn't in that module path on disk, ex. github.com/kubernetes-sigs/controller-runtime vs sigs.k8s.io/controller-runtime. We could still use that module path until we finish working on global configuration (see #1400), and error if the project path doesn't have that module path suffix on-disk.

@joelanford
Copy link
Member

A quick glance of the usages of projutil.CheckAndGetProjectGoPkg() shows we typically use that path for generating import paths in scaffolded files. I could be overlooking something, but in the Go modules world, we no longer care about the on-disk path. The module path is what we'd want to scaffold into the imports, right?

For example, I could create $HOME/projects/memcached-operator with a go.mod file that specifies the module as github.com/joelanford/memcached-operator. The on-disk path is useless for go imports, and I'd want to use the module path everywhere, I think.

The other usage seems to be to build package path strings that we pass to go commands. I'm not exactly sure how that works with go modules, but it could be that we need to use relative paths instead of package paths, e.g.

go build github.com/joelanford/memcached-operator/cmd/manager

becomes

go build ./cmd/manager

@estroz
Copy link
Member

estroz commented May 16, 2019

@joelanford you are correct as long as the project is using modules. However if the project is using dep then the SDK needs to revert back to the current behaviour right?

@joelanford
Copy link
Member

However if the project is using dep then the SDK needs to revert back to the current behaviour right?

Yes

@estroz
Copy link
Member

estroz commented May 20, 2019

#1457 captures the idea of what this PR is trying to fix.

@estroz
Copy link
Member

estroz commented May 23, 2019

Closing in favor of #1475

@estroz estroz closed this May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants