search

coder / sail

Deprecated: Instant, pre-configured VS Code development environments.
https://sail.dev
MIT License
629 stars 36 forks source link

Removed github requirement #226

Open teddy-codes opened 5 years ago

teddy-codes commented 5 years ago

Closes #197

teddy-codes commented 5 years ago

@deansheather Can you have a look at this?

teddy-codes commented 5 years ago

Do you think that just changing the HasPrefix method should simply be changed to Contains?

teddy-codes commented 5 years ago

@deansheather, I am unsure how I would get the path of the repo from the current code... Any ideas?

deansheather commented 5 years ago

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.

deansheather commented 5 years ago

https://gist.github.com/deansheather/af960f705d938f4ef67fa4332d711202

coadler commented 5 years ago

Getting the origin url is probably the best bet.

teddy-codes commented 5 years ago

I think that would work for sure. Will change this when I have a moment

deansheather commented 5 years ago

If we're getting the origin URL from git, I'd suggest something more like: https://gist.github.com/deansheather/d83477514fa0b54cfa04e1c0f4f355c4

@coadler

coadler commented 5 years ago

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.

coadler commented 5 years ago

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.

deansheather commented 5 years ago

Alright, I'm going to apply the first diff then.

deansheather commented 5 years ago

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
coadler commented 5 years ago

Unlucky, didn’t think about that

On Sun, Jun 30, 2019 at 10:33 PM Dean Sheather notifications@github.com 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 .

deansheather commented 5 years ago

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 notifications@github.com wrote:

Unlucky, didn’t think about that

On Sun, Jun 30, 2019 at 10:33 PM Dean Sheather notifications@github.com 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 .