Removed github requirement #226

teddy-codes · 2019-06-21T02:59:59Z

Closes #197

teddy-codes · 2019-06-21T03:02:18Z

@deansheather Can you have a look at this?

deansheather

I think instead of using git -C /path/to/repo remote -v and checking all remotes we should maybe just use the output of git -C /path/to/repo remote get-url origin to only check the origin remote. Thoughts @coadler?

deansheather · 2019-06-21T03:23:52Z

repo.go

@@ -118,6 +137,10 @@ func (r repo) language() string {
 		return ""
 	}

+	if ok := r.isGithubRemote(); !ok {


Should be if !r.isGithubRemote() {

Also this block should be moved to the top of language() before orgRepo = ....

my bad, was tired when putting this together 😄

Also, I would not be able to move this if I included a specific check for the org repo 😛. I think it should stay where it is for now. Once I update the PR, you can have another look.

deansheather · 2019-06-21T03:27:15Z

repo.go

@@ -110,6 +111,24 @@ func parseRepo(defaultSchema, defaultHost, defaultOrganization, name string) (re
 	return r, nil
 }

+func (r repo) isGithubRemote() (ok bool) {
+	cmd := xexec.Fmt("git remote -v")


Shouldn't be using xexec.Fmt here since you're not using a format string.

This won't work unless the current working directory is inside the repository, which isn't guaranteed. You can tell git to use a different path by giving it the -C parameter. Maybe xexec.Fmt is a good idea after all.

deansheather · 2019-06-21T03:32:49Z

repo.go

@@ -110,6 +111,24 @@ func parseRepo(defaultSchema, defaultHost, defaultOrganization, name string) (re
 	return r, nil
 }

+func (r repo) isGithubRemote() (ok bool) {


Should be called isGitHubRemote (capital H).

(ok bool) should just be bool.

Also, instead of logging errors, this function should return them.

deansheather · 2019-06-21T03:36:50Z

repo.go

+	cmd := xexec.Fmt("git remote -v")
+	out, err := cmd.Output()
+	if err != nil {
+		flog.Error("Unable to check repo remotes")


flog.Error("unable to check Git remotes: %s", out)

deansheather · 2019-06-21T03:37:34Z

repo.go

@@ -110,6 +111,24 @@ func parseRepo(defaultSchema, defaultHost, defaultOrganization, name string) (re
 	return r, nil
 }

+func (r repo) isGithubRemote() (ok bool) {
+	cmd := xexec.Fmt("git remote -v")
+	out, err := cmd.Output()


Use cmd.CombinedOutput so we can use this in the error as well.

deansheather · 2019-06-21T03:45:23Z

repo.go

+		return false
+	}
+
+	o := strings.Split(string(out), "\n")


This won't work, as git remote -v is prefixed with the name of each remote.

deansheather · 2019-06-21T03:48:33Z

repo.go

+	o := strings.Split(string(out), "\n")
+
+	for _, url := range o {
+		if strings.HasPrefix(url, "https://github.com") || strings.HasPrefix(url, "[email protected]") {


We should at least know the username/repo of the repository, so I think we should check that the URL matches exactly against what we expect it to be. For example, for Sail it should be https://github.com/cdr/sail.git or [email protected]:cdr/sail.git.

teddy-codes · 2019-06-21T14:28:06Z

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

teddy-codes · 2019-06-21T17:14:56Z

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

deansheather · 2019-06-24T06:22:05Z

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 · 2019-06-24T06:28:17Z

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

coadler · 2019-06-24T16:27:07Z

Getting the origin url is probably the best bet.

teddy-codes · 2019-06-24T19:20:38Z

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

deansheather · 2019-06-25T02:44:19Z

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

@coadler

coadler · 2019-06-28T22:45:34Z

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 · 2019-06-28T22:52:31Z

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 · 2019-07-01T02:59:06Z

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

- This means that only repositories on github.com will have their language autodetected through GitHub API

deansheather · 2019-07-01T03:33:52Z

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 · 2019-07-01T05:44:33Z

Unlucky, didn’t think about that

…

On Sun, Jun 30, 2019 at 10:33 PM Dean Sheather ***@***.***> 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 <#226>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQJ7BZFXI6265KFV2QDC2DP5F3KBANCNFSM4H2IRQ2Q> .

deansheather · 2019-07-01T11:33:15Z

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 ***@***.***> wrote: Unlucky, didn’t think about that On Sun, Jun 30, 2019 at 10:33 PM Dean Sheather ***@***.***> 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 > < #226 >, > 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 <#226>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACVYSVFRNCSGXGXNRR22XN3P5GKUFANCNFSM4H2IRQ2Q> .

Removed github requirement

3cef373

teddy-codes marked this pull request as ready for review June 21, 2019 03:01

deansheather suggested changes Jun 21, 2019

View reviewed changes

Changes based on PR

c9f8fdf

use r.Host to determine repo host in r.language()

931da14

- This means that only repositories on github.com will have their language autodetected through GitHub API

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed github requirement #226

Removed github requirement #226

teddy-codes commented Jun 21, 2019

teddy-codes commented Jun 21, 2019

deansheather left a comment

deansheather Jun 21, 2019

deansheather Jun 21, 2019

teddy-codes Jun 21, 2019

teddy-codes Jun 21, 2019

deansheather Jun 21, 2019

deansheather Jun 21, 2019

deansheather Jun 21, 2019

deansheather Jun 21, 2019

deansheather Jun 21, 2019

deansheather Jun 21, 2019

deansheather Jun 21, 2019

deansheather Jun 21, 2019

deansheather Jun 21, 2019

teddy-codes commented Jun 21, 2019

teddy-codes commented Jun 21, 2019

deansheather commented Jun 24, 2019

deansheather commented Jun 24, 2019

coadler commented Jun 24, 2019

teddy-codes commented Jun 24, 2019

deansheather commented Jun 25, 2019

coadler commented Jun 28, 2019

coadler commented Jun 28, 2019 •

edited

Loading

deansheather commented Jul 1, 2019

deansheather commented Jul 1, 2019

coadler commented Jul 1, 2019 via email

deansheather commented Jul 1, 2019 via email

Removed github requirement #226

Are you sure you want to change the base?

Removed github requirement #226

Conversation

teddy-codes commented Jun 21, 2019

teddy-codes commented Jun 21, 2019

deansheather left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teddy-codes commented Jun 21, 2019

teddy-codes commented Jun 21, 2019

deansheather commented Jun 24, 2019

deansheather commented Jun 24, 2019

coadler commented Jun 24, 2019

teddy-codes commented Jun 24, 2019

deansheather commented Jun 25, 2019

coadler commented Jun 28, 2019

coadler commented Jun 28, 2019 • edited Loading

deansheather commented Jul 1, 2019

deansheather commented Jul 1, 2019

coadler commented Jul 1, 2019 via email

deansheather commented Jul 1, 2019 via email

coadler commented Jun 28, 2019 •

edited

Loading