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

Remove sourcegraph.com vanity import path #27

Open
slimsag opened this Issue Nov 16, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@slimsag
Member

slimsag commented Nov 16, 2018

This package is currently accessible via sourcegraph.com/sourcegraph/go-diff but there isn't really a good reason for this, and we no longer serve vanity import paths at e.g. https://sourcegraph.com/sourcegraph/go-diff (the real location is now https://sourcegraph.com/github.com/sourcegraph/go-diff)

This package is in use by the Go build bots, it looks like: https://github.com/golang/build/blob/master/go.mod#L74

Some firewalls block Cloudflare, which Sourcegraph.com is hosted through -- making this package hard to fetch needlessly. Reported on Slack here: https://gophers.slack.com/archives/C9BMAAFFB/p1542362482359900

A PR for this would be very much appreciated :)

@dmitshur

This comment has been minimized.

Contributor

dmitshur commented Nov 22, 2018

Changing the import path of a package has some cost, which should be taken into account when making a decision like this.

Note that the diff package imports sourcegraph.com/sqs/pbtypes, which is yet another package with a sourcegraph.com/... vanity import path.

There are many more of them, including:

$ go list sourcegraph.com/...
sourcegraph.com/sourcegraph/csp
sourcegraph.com/sqs/pbtypes
sourcegraph.com/sourcegraph/appdash
sourcegraph.com/sourcegraph/appdash/cmd/appdash
sourcegraph.com/sourcegraph/appdash/examples/cmd/webapp
sourcegraph.com/sourcegraph/appdash/examples/cmd/webapp-opentracing
sourcegraph.com/sourcegraph/appdash/httptrace
sourcegraph.com/sourcegraph/appdash/internal/wire
sourcegraph.com/sourcegraph/appdash/opentracing
sourcegraph.com/sourcegraph/appdash/sqltrace
sourcegraph.com/sourcegraph/appdash/traceapp
sourcegraph.com/sourcegraph/appdash/traceapp/tmpl
sourcegraph.com/sourcegraph/datad
sourcegraph.com/sourcegraph/go-diff/cmd/go-diff
sourcegraph.com/sourcegraph/go-diff/diff
sourcegraph.com/sourcegraph/go-git
sourcegraph.com/sourcegraph/go-vcs/cmd/go-vcs
sourcegraph.com/sourcegraph/go-vcs/vcs
sourcegraph.com/sourcegraph/go-vcs/vcs/git
sourcegraph.com/sourcegraph/go-vcs/vcs/gitcmd
sourcegraph.com/sourcegraph/go-vcs/vcs/hg
sourcegraph.com/sourcegraph/go-vcs/vcs/hgcmd
sourcegraph.com/sourcegraph/go-vcs/vcs/internal
sourcegraph.com/sourcegraph/go-vcs/vcs/ssh
sourcegraph.com/sourcegraph/go-vcs/vcs/testing
sourcegraph.com/sourcegraph/go-vcs/vcs/util
sourcegraph.com/sourcegraph/go-vcs/vcs/util/tracer
sourcegraph.com/sourcegraph/gopathexec
sourcegraph.com/sourcegraph/vcsstore
sourcegraph.com/sourcegraph/vcsstore/cmd/vcsstore
sourcegraph.com/sourcegraph/vcsstore/git
sourcegraph.com/sourcegraph/vcsstore/misc
sourcegraph.com/sourcegraph/vcsstore/server
sourcegraph.com/sourcegraph/vcsstore/vcsclient

I'm not opposed to this per-se, I just think it would be helpful to think about the bigger picture and the long term direction before making a change to a single package. For example, removing the vanity import path for this package but not pbtypes will mean that Cloudflare is still a critical component when fetching this package and its dependencies, so that benefit doesn't apply.

@slimsag

This comment has been minimized.

Member

slimsag commented Nov 23, 2018

Yeah, everything you said is true.

Currently @ Sourcegraph we only use two of the packages you've listed:

https://sourcegraph.com/github.com/sourcegraph/sourcegraph@fcee49d4173cadb78b0236a17a72575792ab0f94/-/blob/go.mod#L143-144

My thinking was that the long term goal here would be:

  1. Remove vanity imports for all github.com/sourcegraph packages
  2. Eventually remove support for vanity import paths in Sourcegraph itself

My inclination is that these vanity import paths made sense when sourcegraph.com/<go package path> worked, but they no longer make sense now that <go package path> is actually <full repository name>.

I agree that this would cause an annoyance (need to update the import path) for downstream users of these packages, though. I didn't think that portion through, and I'm not sure the best way to tackle that.

Perhaps what we could do is allow using the package at both locations first, asking users to migrate to github.com, and then enforcing the github.com import with an // import comment later.

@dmitshur

This comment has been minimized.

Contributor

dmitshur commented Nov 23, 2018

My thinking was that the long term goal here would be:

  1. Remove vanity imports for all github.com/sourcegraph packages
  2. Eventually remove support for vanity import paths in Sourcegraph itself

That makes sense as a long term vision, thanks for explaining.

Perhaps what we could do is allow using the package at both locations first, asking users to migrate to github.com, and then enforcing the github.com import with an // import comment later.

A gradual transition like that makes sense, but it's worth noting that it doesn't change the amount of work that needs to be done, it just makes it possible to do that work over a longer period of time without immediate breakage.

When the // import comment is removed, the README should be updated to be very clear about the canonical import path, so users don't have to guess which of multiple import paths is the one that has best support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment