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

x/tools/gopls: respect GOPRIVATE setting when linking to pkg.go.dev (hover, documentLink) #36998

Closed
stamblerre opened this issue Feb 3, 2020 · 14 comments

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Feb 3, 2020

Reported on Slack. We shouldn't over links to packages covered by GOPRIVATE.

@hyangah
Copy link
Contributor

@hyangah hyangah commented Feb 3, 2020

Just want mention a different use case: organizations that run their internal doc servers would want the linkTarget to point to their doc servers. OTOH, I expect they can easily set up private proxies to avoid depending on GOPRIVATE.

@gopherbot
Copy link

@gopherbot gopherbot commented May 13, 2020

Change https://golang.org/cl/233524 mentions this issue: internal/lsp/source: don't link to packages matching GOPRIVATE in hover

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented May 14, 2020

There's actually one more case where this needs to be handled - https://pkg.go.dev links get generated for import statements through the textDocument/documentLink request.

gopherbot pushed a commit to golang/tools that referenced this issue May 15, 2020
Currently, our hover text by default links point to public documentation
sites (e.g. pkg.go.dev). This doesn't make sense for private repos, so
hide the hovertext link when the import path matches GOPRIVATE.

Implementing this was a little messy. To be optimal I had to thread
the value of goprivate through cache.view, and to be correct I had to
duplicate some code from cmd/go internal.

Regtest will follow after https://golang.org/cl/232983 is submitted.

Updates golang/go#36998

Change-Id: I1e556471bf919fea30132d9642426a08fdb7f434
Reviewed-on: https://go-review.googlesource.com/c/tools/+/233524
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@heschik
Copy link
Contributor

@heschik heschik commented May 27, 2020

I think this is fixed?

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented May 27, 2020

Let's rename the issue to cover the document link case as well.

@stamblerre stamblerre changed the title x/tools/gopls: respect GOPRIVATE setting for links to documentation on hover x/tools/gopls: respect GOPRIVATE setting when linking to pkg.go.dev (hover, documentLink) May 27, 2020
@findleyr
Copy link
Contributor

@findleyr findleyr commented May 27, 2020

Yeah, I will get to the documentLink eventually. Can leave this assigned to me.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 15, 2020

Change https://golang.org/cl/237938 mentions this issue: internal/lsp/regtest: add a regtest for hover handling of GOPRIVATE

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 16, 2020

Change https://golang.org/cl/238029 mentions this issue: internal/lsp: honor GOPRIVATE in documentLinks and go.mod hovers

gopherbot pushed a commit to golang/tools that referenced this issue Jun 16, 2020
Add a regtest to verify that GOPRIVATE identifiers are not given a link
to pkg.go.dev. For efficiency, as well as to exercise dynamic
configuration, do all this in a single regtest.

Updates golang/go#36998

Change-Id: I9102a11312db5c334fdbd30cce9ca2d2e19e9ac2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/237938
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@stamblerre stamblerre modified the milestones: gopls/v0.5.0, gopls/v0.4.2 Jun 18, 2020
@sandipb
Copy link

@sandipb sandipb commented Jun 22, 2020

Is it an additional bug that all this pkg.go.dev hover and link activity is still going on even when GO111MODULE is set to off in the environment? Isn't this a Go modules feature?

@findleyr
Copy link
Contributor

@findleyr findleyr commented Jun 22, 2020

@sandipb: yes, GOPRIVATE relates to proxy behavior when using modules. This bug is about hiding links to package documentation when paths match GOPRIVATE. If paths are private, it's likely that those links will be broken anyway.

But as you say, that only applies to module mode. Are you saying that if GO111MODULE=off, we shouldn't link to package documentation? Or are you saying if GO111MODULE=off we shouldn't check GOPRIVATE? There may be a bug, I'm not sure I understand your question.

@sandipb
Copy link

@sandipb sandipb commented Jul 2, 2020

Hi @findleyr sorry for the delay in the response.

I meant, if GO111MODULE=off, regardless of the value of GOPRIVATE, are links pointing to proxies like pkg.dev expected? I assumed that pkg.dev based hosted docs was part of the module infrastructure.

@findleyr
Copy link
Contributor

@findleyr findleyr commented Jul 2, 2020

Hi @sandipb -- both godoc.org and pkg.go.dev host package documentation. They are not proxies. Unlike godoc.org, pkg.go.dev is "module-aware", so you can see package documentation for older module versions.

In GOPATH mode, it still seems reasonable to link to package documentation for the package import path. In any case, if I recall correctly we link to the latest version of documentation, so whether using modules or not the linked package documentation might be for a newer version than what you are importing.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Jul 2, 2020

Agree with everything @findleyr said here, but wanted to add a quick point of clarification on the last statement:

In any case, if I recall correctly we link to the latest version of documentation, so whether using modules or not the linked package documentation might be for a newer version than what you are importing.

We do link to the correct version of the docs (code here), so if you are using modules, you will get the correct versions.

However, as @findleyr said, even if you are not using modules, these documentation links to https://pkg.go.dev are still useful.

@sandipb
Copy link

@sandipb sandipb commented Jul 2, 2020

Ack. Thanks, folks, for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.