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

Jump to bottom

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
Closed

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

stamblerre opened this issue Feb 3, 2020 · 14 comments
Labels
Documentation FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.4.2

Comments

@stamblerre
Copy link
Contributor

Reported on Slack. We shouldn't over links to packages covered by GOPRIVATE.
@stamblerre stamblerre added this to the gopls/v0.4.0 milestone Feb 3, 2020
@gopherbot gopherbot added Documentation Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Feb 3, 2020
@hyangah
Copy link
Contributor

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.

@stamblerre stamblerre mentioned this issue Feb 11, 2020
Closed
@stamblerre stamblerre added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 26, 2020
@stamblerre stamblerre modified the milestones: gopls/v0.4.0, gopls/v0.5.0 Apr 2, 2020
@findleyr findleyr self-assigned this May 13, 2020
@gopherbot
Copy link

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

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
@findleyr
internal/lsp/source: don't link to packages matching GOPRIVATE in hover
8ddc067 
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>
@heschi
Copy link
Contributor

heschi commented May 27, 2020

I think this is fixed?

@stamblerre
Copy link
Contributor Author

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

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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
@findleyr
internal/lsp/regtest: add a regtest for hover handling of GOPRIVATE
b1f3cdd 
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 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

@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.

@stamblerre stamblerre mentioned this issue Jun 24, 2020
Closed
@sandipb
Copy link

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 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

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 commented Jul 2, 2020

Ack. Thanks, folks, for the clarification.

@golang golang locked and limited conversation to collaborators Jul 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.4.2
Development

No branches or pull requests
6 participants
@sandipb @hyangah @stamblerre @gopherbot @heschi @findleyr