allow github reporter to work with git@ urls #3148

dragon512 · 2017-04-25T19:22:12Z

Allow GitHub to parse https:// and git@github.com:xxx style repository URLS correctly
Add some verbose messages that can be useful.

Contributor Checklist:

I have updated the unit tests
I have created a file in the master/buildbot/newsfragment directory (and read the README.txt in that directory)
I have updated the appropriate documentation

mention-bot · 2017-04-25T19:22:14Z

@dragon512, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hcwiley, @tardyp and @anish to be potential reviewers.

codecov · 2017-04-25T20:50:55Z

Codecov Report

Merging #3148 into master will increase coverage by <.01%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #3148      +/-   ##
==========================================
+ Coverage   88.25%   88.26%   +<.01%     
==========================================
  Files         323      323              
  Lines       33712    33715       +3     
==========================================
+ Hits        29753    29758       +5     
+ Misses       3959     3957       -2

Impacted Files	Coverage Δ
master/buildbot/reporters/github.py	`93.81% <85.71%> (+2.32%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3c6532...bf6f0fc. Read the comment docs.

tardyp · 2017-04-26T08:09:56Z

master/buildbot/reporters/github.py

            repoOwner, repoName = project.split('/')
-        else:
+        # Given we have a repository define try to get the owner and name
+        elif sourcestamps[0]['repository']:


Please install codecov chrome extension or look at the report.
We require that those kind of changes are unit tested before they can be included in the code

Maybe using the project property is safe since github changes coming in via the www hook or PullrequestPoller have the project property set to the correct value only timed/force schedulers would have to set that property.

tardyp · 2017-04-26T08:10:47Z

master/buildbot/reporters/github.py

@@ -94,10 +95,16 @@ def createStatus(self,
        if context is not None:
            payload['context'] = context

-        return self._http.post(
+        if self.verbose:


I guess this is a separate feature which belongs to a separate PR. I think this would be better to put it in HttpBase reporter

tardyp · 2017-04-26T08:17:53Z

master/buildbot/reporters/github.py

-            repoOwner = repo[0]
-            repoName = '.'.join(repo[1].split('.')[:-1])
+            # if this repo is in the form of git@ not http://
+            if sourcestamps[0]['repository'].startswith("git@"):


I think this algorithm is useful not only for github, but also for bitbucket, gitlab, and all github clones.

We should factorize it in an utility function in (create new buildbot.util.repository_urls module)

def getUserAndProjectFromRepoUrl(repoUrl): [...]

Frodox · 2017-06-09T10:11:44Z

Looks like it should fix #3215
Ping @dragon512 @samvasko

P.s. take a look #3305

dragon512 · 2017-06-09T14:35:26Z

Cool, I should pull this change in and redo the fix with this new util.

tardyp · 2017-06-09T14:42:05Z

@dragon512 please do.

Frodox · 2017-06-14T19:59:53Z

master/buildbot/reporters/github.py

@@ -205,5 +216,6 @@ def createStatus(self,
        payload = {'body': description}

        return self._http.post(
-            '/'.join(['/repos', repo_user, repo_name, 'issues', issue, 'comments']),
+            '/'.join(['/repos', repo_user, repo_name,


What is the goal of such wrapping here, and leave this one as is? (Since it is less 80 characters anyway)

I formatted with autopep8. It seems to want to do this. I don't know why. I switch it back and did the format again .. this went back to the two line statement.

I can see about making the line length longer and seeing it this will go away

Ya I just tried again with --max-line-length 132
It was still reformatting this line. I can manually set it back

dragon512 · 2017-07-26T21:36:19Z

@tardyp I am unclear how this is failing worse. I rebased. I have a new test added to cover the basic three lines that had not been covered before. How is this reporting even worse coverage than before?

tardyp · 2017-07-26T21:47:20Z

master/buildbot/reporters/github.py

+            repoName = giturl.repo
+
+        if self.verbose:
+            log.msg("Updating github status: repoOwner={repoOwner}, repoName={repoName}".format(


codecov says this line is not covered. that's fine to me. just fix the doc issue.

dragon512 · 2017-07-26T22:17:38Z

there is no real doc change, as this fixes the reporter to correct consume json data with git@ formatted URLs. this is more of a bug fix, vs a API change

Frodox · 2017-07-27T06:20:21Z

@dragon512 @tardyp means doc issue, i.e. it doesn't build because of spelling. Take a look test

relnotes/index.rst:33:RepoName:["Forename", "Repairmen", "Namedrop", "Reamonn", "Repentance", "Repiner"]
relnotes/index.rst:62:RepoName:["Forename", "Repairmen", "Namedrop", "Reamonn", "Repentance", "Repiner"]

Frodox · 2017-07-27T06:23:08Z

master/buildbot/newsfragments/github.feature

@@ -0,0 +1 @@
+GitHubStatusPush now support reporting to ssh style URLs, ie git@github.com:Owner/RepoName.git


Could you please add link on GitHubStatusPush, format it with backtricks, and backtricks for git@github... (mark it as code). And looks like spell check doesn't like this RepoName. Or maybe later one. Anyway, you can add it to ignore list

Thanks! I think I corrected it properly

Frodox · 2017-07-27T06:28:45Z

master/buildbot/reporters/github.py

@@ -136,12 +137,16 @@ def send(self, build):
        else:
            issue = None

-        if project:
+        if "/" in project:


what is it for? who can set project to smth like repoOwner/repoName ?

This was for when I set project to "foobar" not "owner/repo" This protects the code and allow the code to use the meta data that was really sent to get the data. There is no documentation other than that the project is a meta value. there is nothing saying that it can have magic values. I been burned by this a few times.

Add some verbose messages that can be useful add a release note fragment add test code

dragon512 · 2017-07-27T15:53:35Z

@tardyp I think I have it. Sorry for the delay on finishing this.

dragon512 · 2017-08-03T21:42:23Z

@tardyp Just a friendly ping. Is there anything holding back this PR from being merged?

tardyp requested changes Apr 26, 2017

View reviewed changes

tardyp added needs tests needs relnotes labels Apr 26, 2017

tardyp reviewed Apr 26, 2017

View reviewed changes

dragon512 force-pushed the github-reporter-fix branch 3 times, most recently from 3b7d6b0 to 580eeb8 Compare June 14, 2017 18:52

Frodox reviewed Jun 14, 2017

View reviewed changes

dragon512 force-pushed the github-reporter-fix branch 2 times, most recently from fae5711 to 60967c3 Compare July 26, 2017 21:17

tardyp reviewed Jul 26, 2017

View reviewed changes

Frodox reviewed Jul 27, 2017

View reviewed changes

allow github reporter to work with git@ urls

bf6f0fc

Add some verbose messages that can be useful add a release note fragment add test code

dragon512 force-pushed the github-reporter-fix branch from fcb0e7d to bf6f0fc Compare July 27, 2017 15:09

tardyp merged commit 66bcb73 into buildbot:master Aug 8, 2017

anish mentioned this pull request Oct 7, 2017

Minor fix : On rebuild, giturl is empty. Don't backtrace out #3682

Closed

p12tic mentioned this pull request Jan 3, 2021

Github status push does not process git url correctly #3215

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow github reporter to work with git@ urls #3148

allow github reporter to work with git@ urls #3148

dragon512 commented Apr 25, 2017 •

edited

mention-bot commented Apr 25, 2017

codecov bot commented Apr 25, 2017 •

edited

tardyp Apr 26, 2017

noc0lour May 11, 2017

tardyp Apr 26, 2017

tardyp Apr 26, 2017

Frodox commented Jun 9, 2017 •

edited

dragon512 commented Jun 9, 2017

tardyp commented Jun 9, 2017

Frodox Jun 14, 2017

dragon512 Jun 14, 2017

dragon512 Jun 14, 2017

dragon512 commented Jul 26, 2017

tardyp Jul 26, 2017

dragon512 commented Jul 26, 2017

Frodox commented Jul 27, 2017 •

edited

Frodox Jul 27, 2017 •

edited

dragon512 Jul 27, 2017

Frodox Jul 27, 2017

dragon512 Jul 27, 2017

dragon512 commented Jul 27, 2017

dragon512 commented Aug 3, 2017

		@@ -0,0 +1 @@
		GitHubStatusPush now support reporting to ssh style URLs, ie git@github.com:Owner/RepoName.git

allow github reporter to work with git@ urls #3148

allow github reporter to work with git@ urls #3148

Conversation

dragon512 commented Apr 25, 2017 • edited

Contributor Checklist:

mention-bot commented Apr 25, 2017

codecov bot commented Apr 25, 2017 • edited

Codecov Report

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

Frodox commented Jun 9, 2017 • edited

dragon512 commented Jun 9, 2017

tardyp commented Jun 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dragon512 commented Jul 26, 2017

Choose a reason for hiding this comment

dragon512 commented Jul 26, 2017

Frodox commented Jul 27, 2017 • edited

Frodox Jul 27, 2017 • edited

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

dragon512 commented Jul 27, 2017

dragon512 commented Aug 3, 2017

dragon512 commented Apr 25, 2017 •

edited

codecov bot commented Apr 25, 2017 •

edited

Frodox commented Jun 9, 2017 •

edited

Frodox commented Jul 27, 2017 •

edited

Frodox Jul 27, 2017 •

edited