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

Parse git URLs in a more generic way instead of tight coupling against github.com #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

skymeyer
Copy link

@skymeyer skymeyer commented Sep 3, 2018

Fixes #3

@coveralls
Copy link

coveralls commented Sep 3, 2018

Coverage Status

Coverage remained the same at 9.649% when pulling c95fe89 on skymeyer:genericgit into b3920ba on bramp:master.

@bramp
Copy link
Owner

bramp commented Sep 3, 2018

The reason it was specific to GitHub, is because I didn't think you could always assume the git and https URLs mapped like this. It seems to work for bitbucket, do you know if this mapping of URLs is part of a standard or spec?

@skymeyer
Copy link
Author

skymeyer commented Sep 3, 2018

Just tested gitlab and that seems to work as well for me.

The only spec I know of is what is described in https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols. If a git server implements both ssh and https protocol it is assumed that both are served from the same hostname, so I think this generic approach should be ok.

Otherwise we can resort to creating a parser interface (supplying some defaults for github, bitbucket, gitlab, ...) and allowing injection in the redirectCreator for people having a more exotic use-case or private git servers from which we can't derive the parser from the remote URL.

@bramp
Copy link
Owner

bramp commented Sep 3, 2018

Thanks for researching this. Ok I'm happy with the current changes. If you wish to create a parsing/mapping interface feel free, or we could just defer that to later.

@skymeyer
Copy link
Author

skymeyer commented Sep 4, 2018

I'd defer that to later yes if we have concrete short comings, I think we are good like it is for now.

@@ -14,7 +14,7 @@

// goredirects creates static HTML that redirects go packages hosted
// on a vanity domain to GitHub.
package main // import "bramp.net/goredirects"
package main
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be reverted

if len(matches) == 3 {
return fmt.Sprintf("https://github.com/%s/%s.git", matches[1], matches[2])
// gitSSHtoHTTPS takes a URL to a SSH repo, and returns the equlivant HTTPS url.
// If it is already a valid Git HTTPS URL just return it.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth documenting the assumption we are making about the URL.

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

Successfully merging this pull request may close these issues.

None yet

3 participants