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/go/vcs: vcs.go does not support private repositories like GitHub Enterprise & GitLab. It is too restrictive because of regex pattern validation #21465

Open
tariq1890 opened this Issue Aug 16, 2017 · 18 comments

Comments

Projects
None yet
7 participants
@tariq1890
Copy link

tariq1890 commented Aug 16, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.8.3

What operating system and processor architecture are you using (go env)?

GOHOSTOS = darwin GOARCH = amd64

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

What did you expect to see?

We are unable to use the VCS tool with GitHub Enterprise URLs, GitLab URLs etc. These are git repositories and they need to be supported

What did you see instead?

GitHub Enterprise, GitLab Enterprise URLs being supported.

I have an idea for this. Instead of regex validation, we can loop through the various known VCS PingCmds to determine the type of repository. When a positive result is received, we break out of the loop and continue repository processing based on the determined repository type. If you're okay with this idea, I am ready to make a PR.

@tariq1890 tariq1890 changed the title vcs.go does not support private repositories. It is too restrictive because of regex pattern validation vcs.go does not support private repositories like GitHub Enterprise & GitLab. It is too restrictive because of regex pattern validation Aug 16, 2017

@davecheney

This comment has been minimized.

Copy link
Contributor

davecheney commented Aug 16, 2017

@tariq1890

This comment has been minimized.

Copy link
Author

tariq1890 commented Aug 16, 2017

Sample URLs are :
github.myenterprise.org/author/repo1
github.myenterprise.com/author/repo2

These URLs will fail the checks enforced by vcs.go. Have a look at this code snippet for instance,
L(601-608) in vcs.go
// Github
{
prefix: "github.com/",
re: ^(?P<root>github\.com/[A-Za-z0-9_.\-]+/[A-Za-z0-9_.\-]+)(/[A-Za-z0-9_.\-]+)*$,
vcs: "git",
repo: "https://{root}",
check: noVCSSuffix,
},

To make it work for our GHE we had to patch vcs.go to add our GitHub Enterprise prefix and URL pattern. This is not the best solution IMO

@dcheney-atlassian

This comment has been minimized.

Copy link

dcheney-atlassian commented Aug 16, 2017

Please try appending .git to the url. Does that work?

@tariq1890

This comment has been minimized.

Copy link
Author

tariq1890 commented Aug 16, 2017

I tried it. It does not work.

I have looked into the code. Our GHE URLs fail the prefix test let alone the URL regex.

Here the expected prefix is github.com, while our company's GHE URLs have the following prefix :

github.enterprise_name.org.

A company I had worked in earlier had github.company_name.com.

I am sorry for not disclosing the complete name as I am bound by NDA. But this information should suffice I guess.

@sebito91

This comment has been minimized.

Copy link

sebito91 commented Aug 16, 2017

This is pretty common, going through this myself to be honest. But poke a bit further around and you find the workaround that actually does do the needful: #19251 --> https://gist.github.com/shurcooL/6927554

Setting up the recommendation from @shurcooL does work, it's a bit annoying but fine.

@sebito91

This comment has been minimized.

Copy link

sebito91 commented Aug 16, 2017

Also, @tariq1890 your code is most likely going to hit this branch of the vcsPaths checker:

 896     },
 897
 898     // General syntax for any server.
 899     // Must be last.
 900     {
 901         re:   `^(?P<root>(?P<repo>([a-z0-9.\-]+\.)+[a-z0-9.\-]+(:[0-9]+)?(/~?[A-Za-z0-9_.\-]+)+?)\.(?P<vcs>bzr|git|hg|svn))(/~?[A-Za-z0-9_.\-]+)*$`,
 902         ping: true,
 903     },

For my GHE setup, our URL is git...com and we trigger the last entry in the list. I suspect your site is doing the same, and if that's indeed the case then the links above should do the trick for you.

@dcheney-atlassian

This comment has been minimized.

Copy link

dcheney-atlassian commented Aug 16, 2017

When you say

I tried it. It does not work.

It's really helpful if you can include the command you typed and the output you received. We want to help, but we can't see your screen.

@tariq1890

This comment has been minimized.

Copy link
Author

tariq1890 commented Aug 16, 2017

Alright. More details.

So the method that is invoked is RepoRootForImportPath in vcs.go

and this is the error emitted

unrecognized import path "github.<company_name>.org/<org_name>/<repo_name>"

My point is that I am able to get this working by hardcoding my GHE URL prefix and pattern to the vcsPaths array. But this is a bad solution.

I am asking that we reconsider the validation method for the repository URLs. Please refer to my first post, I have provided my idea there. I believe with this idea implemented, it wouldn't cause backward compatibility issues either.

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Aug 16, 2017

What @dcheney-atlassian said was:

Please try appending .git to the url. Does that work?

Did you try that? What was the command you ran? Was it something like this?

go get -u github.<company_name>.org/<org_name>/<repo_name>.git

Can you post the full command and its output from your terminal (you can replace company/org/repo names of course)?

@tariq1890

This comment has been minimized.

Copy link
Author

tariq1890 commented Aug 16, 2017

@shurcooL I am not trying the go get command. In fact, I am not trying any go command here. We have other workarounds for go get issue.

I am talking about the vcs.go source file in go tools. vcs.go seems to be intended as a utility library to assist with the interacting with version control repos. The way it is coded right now, we are unable to use it for interacting with our private repositories.

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Aug 16, 2017

I see. You're referring to vcs.RepoRootForImportPath. I'll update the issue title to reflect that.

Can you post the code that you used then? The same question applies, what was the import path you used; did it have a ".git" suffix?

@dmitshur dmitshur changed the title vcs.go does not support private repositories like GitHub Enterprise & GitLab. It is too restrictive because of regex pattern validation x/tools/go/vcs: vcs.go does not support private repositories like GitHub Enterprise & GitLab. It is too restrictive because of regex pattern validation Aug 16, 2017

@gopherbot gopherbot added this to the Unreleased milestone Aug 16, 2017

@tariq1890

This comment has been minimized.

Copy link
Author

tariq1890 commented Aug 16, 2017

Yes. Correct. Thanks for the Issue edit.

Yes i tried with .git suffix as well. I get the same unrecognized import path error

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Aug 16, 2017

I can't reproduce.

$ goexec 'vcs.RepoRootForImportPath("github.examplecompany.org/org/repo.git", false)'
(*vcs.RepoRoot)(&vcs.RepoRoot{
	VCS: (*vcs.Cmd)(&vcs.Cmd{
		Name:        (string)("Git"),
		Cmd:         (string)("git"),
		CreateCmd:   (string)("clone {repo} {dir}"),
		DownloadCmd: (string)("pull --ff-only"),
		TagCmd: ([]vcs.TagCmd)([]vcs.TagCmd{
			(vcs.TagCmd)(vcs.TagCmd{
				Cmd:     (string)("show-ref"),
				Pattern: (string)("(?:tags|origin)/(\\S+)$"),
			}),
		}),
		TagLookupCmd: ([]vcs.TagCmd)([]vcs.TagCmd{
			(vcs.TagCmd)(vcs.TagCmd{
				Cmd:     (string)("show-ref tags/{tag} origin/{tag}"),
				Pattern: (string)("((?:tags|origin)/\\S+)$"),
			}),
		}),
		TagSyncCmd:     (string)("checkout {tag}"),
		TagSyncDefault: (string)("checkout master"),
		LogCmd:         (string)(""),
		Scheme: ([]string)([]string{
			(string)("git"),
			(string)("https"),
			(string)("http"),
			(string)("git+ssh"),
		}),
		PingCmd: (string)("ls-remote {scheme}://{repo}"),
	}),
	Repo: (string)("github.examplecompany.org/org/repo"),
	Root: (string)("github.examplecompany.org/org/repo.git"),
})
(interface{})(nil)

Are you using the latest version of golang.org/x/tools/go/vcs package?

@tariq1890

This comment has been minimized.

Copy link
Author

tariq1890 commented Aug 16, 2017

I am using Go 1.8.3. I guess you're trying with ".git suffix. If we go with that approach, then normal GitHub repos won't work. We get invalid suffix error when we suffix github repository paths with .git.

@tariq1890

This comment has been minimized.

Copy link
Author

tariq1890 commented Aug 16, 2017

Can you try go exec with an existing GitHub repository suffixed with .git ?

Try this ?

github.com/golang/go.git

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Aug 16, 2017

Yes, you don't need .git suffix for normal github.com/user/repo Go packages. You said the issue was with GitHub Enterprise repos. Only those need .git suffix in their import path.

The .git suffix is needed to tell Go that it's a git repo. It can't know whether a given URL is GitHub Enterprise or not. But "github.com" URLs are well known, that's why they can be treated specially and not require .git suffix.

You can read more about this at https://golang.org/cmd/go/#hdr-Remote_import_paths.

@tariq1890

This comment has been minimized.

Copy link
Author

tariq1890 commented Aug 16, 2017

But this would mean suffixing GHE and GitLab URLs with .git . This does kinda fall short (hacky) from an API/Library standpoint.

But I do understand now that vcs.go has been written keeping the go get command in mind. Thanks for your time.

I'll proceed with writing a custom library. I strongly feel that ping commands should be used for validation here rather than regex.

@amnonbc

This comment has been minimized.

Copy link

amnonbc commented Feb 9, 2018

The problem with suffixing the URLs with .git is that this changes their import name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.