sail
sail copied to clipboard
coder
Removed github requirement
Do you think that just changing the HasPrefix method should simply be changed to Contains?
@deansheather, I am unsure how I would get the path of the repo from the current code... Any ideas?
Actually, I just had another look at repo.go. Why can't we just use r.Host to check if the repository is on GitHub? That would simplify this PR significantly.
I think that would work for sure. Will change this when I have a moment
If we're getting the origin URL from git, I'd suggest something more like: https://gist.github.com/deansheather/d83477514fa0b54cfa04e1c0f4f355c4
@coadler
The diff gist is a bit hard to parse, but url.Parse would definitely be better here, and your diff seems to handle the : correctly.
Also, I may have misled you. Reread through the comments and my
Getting the origin url is probably the best bet.
comment was in response to the PR comment you pinged me in and I didn't notice the comment before it pointing out r.Host.
r.Host should solve this problem, my apologies. I made sure to handle all edge cases I could find in parseRepo so it should be robust enough to use.
One issue I just found with r.Host is that it's not correct if you do something like:
sail run gitlab.com/deansheather/some-repo => r.Host is gitlab.com
sail run deansheather/some-repo => r.Host is github.com even though it's already cloned from another host
Unlucky, didn’t think about that
On Sun, Jun 30, 2019 at 10:33 PM Dean Sheather [email protected] wrote:
One issue I just found with r.Host is that it's not correct if you do something like:
sail run gitlab.com/deansheather/some-repo => r.Host is gitlab.com sail run deansheather/some-repo => r.Host is github.com even though it's already cloned from another host
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cdr/sail/pull/226?email_source=notifications&email_token=ABQJ7B545QBMI654RX6GLJDP5F3KBA5CNFSM4H2IRQ22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY44UWY#issuecomment-507103835, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQJ7BZFXI6265KFV2QDC2DP5F3KBANCNFSM4H2IRQ2Q .
I think the best way to fix this is probably to add the git remote get-url origin check to parseRepo() if the repo is already on disk. Problem with
that, though, is that parseRepo doesn't know where to look for the
repository, so we'll have to add even more params.
On Mon, Jul 1, 2019 at 5:44 AM Colin [email protected] wrote:
Unlucky, didn’t think about that
On Sun, Jun 30, 2019 at 10:33 PM Dean Sheather [email protected] wrote:
One issue I just found with r.Host is that it's not correct if you do something like:
sail run gitlab.com/deansheather/some-repo => r.Host is gitlab.com sail run deansheather/some-repo => r.Host is github.com even though it's already cloned from another host
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/cdr/sail/pull/226?email_source=notifications&email_token=ABQJ7B545QBMI654RX6GLJDP5F3KBA5CNFSM4H2IRQ22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY44UWY#issuecomment-507103835 , or mute the thread < https://github.com/notifications/unsubscribe-auth/ABQJ7BZFXI6265KFV2QDC2DP5F3KBANCNFSM4H2IRQ2Q
.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cdr/sail/pull/226?email_source=notifications&email_token=ACVYSVFANCBW5P7XHMAOLJDP5GKUFA5CNFSM4H2IRQ22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY5BQII#issuecomment-507123745, or mute the thread https://github.com/notifications/unsubscribe-auth/ACVYSVFRNCSGXGXNRR22XN3P5GKUFANCNFSM4H2IRQ2Q .