changes: add additional information to properties in GitHubPullrequestPoller by noc0lour · Pull Request #3074 · buildbot/buildbot

noc0lour · 2017-03-25T06:21:35Z

I want to checkout if this would have chances to get included into the main tree once I'm done refining it.
Basically I'd like to extract more information from the PR and add it to the properties which then can be used in the builders.

Yes, no? Or in a different way?

mention-bot · 2017-03-25T06:21:35Z

@noc0lour, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DaulPavid to be a potential reviewer.

noc0lour · 2017-03-25T08:17:17Z

@tardyp this is more a "design" question I guess, if we allow additional special properties to propagate from different source steps.

tardyp · 2017-03-25T08:19:14Z

You can look at the gerrit change source source code, it has similar code.
It does create one property per event attribute (recursively e.g
{
author: {'email': 'foo@bar'}
}
will create a property 'gerrit_event.author.email'

noc0lour · 2017-03-25T08:22:36Z

A cool, I'll have a look, probably I want to flatten them as well. For the GH PR information I don't want to create the properties automatically as this would pollute them with too much stuff.

tardyp · 2017-03-25T09:42:28Z

indeed, this is a mistake that I made for gerrit design, we should have a whitelist configuration in the changesource (maybe fnmatch based).

codecov · 2017-03-26T03:52:46Z

Codecov Report

Merging #3074 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #3074      +/-   ##
=========================================
+ Coverage   88.29%   88.3%   +0.01%     
=========================================
  Files         314     314              
  Lines       32926   32959      +33     
=========================================
+ Hits        29071   29104      +33     
  Misses       3855    3855

Impacted Files	Coverage Δ
master/buildbot/changes/github.py	`100% <100%> (ø)`	⬆️
master/buildbot/www/hooks/github.py	`92.85% <100%> (+0.48%)`	⬆️
master/buildbot/www/authz/endpointmatchers.py	`93.79% <0%> (+0.04%)`	⬆️

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 673afe8...bb728e1. Read the comment docs.

noc0lour · 2017-03-26T20:22:59Z

I'd like to have the parsing of PRs in the webhook the same as it is in this PR poller. (e.g. add these changes there as well :) ). Should I just copy & paste ?

seankelly · 2017-03-27T01:28:04Z

Yes. The poller and webhook should stay in sync for what features they support.

tardyp · 2017-03-27T08:06:29Z

master/buildbot/changes/github.py

+        "github.mergeable",
+        "github.mergeable_state",
+        "github.merged_by",
+    ]


looks like a lot of properties for the defaults.
This should be configurable

tardyp · 2017-03-27T08:07:10Z

master/buildbot/changes/github.py

+                if not any([
+                        prop.startswith(name)
+                        for prop in self.GH_PROPERTY_WHITELIST
+                ]):


I would profer is you used fnmatch https://docs.python.org/2/library/fnmatch.html

I looked into fnmatch but it didn't seem to provide a lot of flexibility if I use a preconfigured list. I'll look into making the list of properties configurable.

tardyp · 2017-03-27T08:12:00Z

master/buildbot/www/hooks/github.py

        return payload

+    def _get_PR_information(self, pr_info):
+        def flatten(properties, base, info_dict):


I dont like to copy the exact same code in two different places. maybe we should have it in a changes/github.py mixin class

tardyp · 2017-03-27T18:12:19Z

I was thinking of a list of fnmatch expr Le lun. 27 mars 2017 19:59, Andrej Rode <notifications@github.com> a écrit :

…

***@***.**** commented on this pull request. ------------------------------ In master/buildbot/changes/github.py <#3074 (comment)>: > @@ -171,6 +198,24 @@ def _getStateObjectId(self): '%s/%s' % (self.owner, self.repo), self.db_class_name) defer.returnValue(result) + def _parseGitHubPR(self, pr_info): + def flatten(properties, base, info_dict): + for k, v in iteritems(info_dict): + name = ".".join([base, k]) + if not any([ + prop.startswith(name) + for prop in self.GH_PROPERTY_WHITELIST I looked into fnmatch but it didn't seem to provide a lot of flexibility if I use a preconfigured list. I'll look into making the list of properties configurable. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3074 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGtIzDbXN6yKtez1jxNmMriXYE3gqXIks5rp3I_gaJpZM4Mo-5r> .

noc0lour · 2017-03-29T03:11:00Z

I think the drawback of having a fnmatch expression is the whole tree has to be flattened first before it can be applied. Otherwise it couldn't match subtrees like github.base. or we match backwards (e.g. fnmatch.match(desired_key, key+"*") but then we don't have any advantages over using startswith.
if you are ok with first flatten the whole file and then matching desired entries (I'd prefer re over fnmatch anytime).

noc0lour · 2017-04-06T07:04:05Z

I have no idea why the coverage suddenly drops in files I did not even touch? And random things fail in the CI tests? @tardyp ? All files I touched have the same or better coverage.

tardyp · 2017-04-06T08:01:50Z

@noc0lour its because py3.5 tests fail, thus the coverage of py3 is not taken in account

tardyp

looks good! need to fix the py3 tests though

tardyp · 2017-04-06T08:21:46Z

master/buildbot/test/unit/test_changes_github.py

        self.assertEqual(change['repository'],
                         'https://github.com/defunkt/buildbot.git')
        self.assertEqual(change['files'], ['README.md'])
+        self.assertDictContainsSubset(_GH_PARSED_PROPS, change['properties'])


this is deprecated in py3.2 (and we dont allow warnings)

https://docs.python.org/3/whatsnew/3.2.html?highlight=assertdictcontainssubset

you probably will need to replace that with a for k,v in _GH_PARSED_PROPS.items()

tardyp · 2017-04-06T08:22:45Z

master/buildbot/test/unit/test_changes_github.py

        self.assertEqual(change['repository'],
                         'https://github.com/defunkt/buildbot.git')
        self.assertEqual(change['files'], ['README.md'])
+        self.assertDictContainsSubset(_GH_PARSED_PROPS, change['properties'])


tardyp · 2017-04-06T08:28:46Z

master/buildbot/changes/github.py

+                name = ".".join([base, k])
+                if isinstance(v, dict):
+                    flatten(properties, name, v)
+                elif any([fnmatch(name, expr)


noc0lour · 2017-04-06T17:13:42Z

Yay, codecov is happy again :)

noc0lour · 2017-04-12T16:27:26Z

anything left :) ?

noc0lour force-pushed the extent_gh_information branch from 26cf8e5 to ee917a1 Compare March 26, 2017 03:38

noc0lour changed the title ~~feedback: Extent gh information~~ changes: add additional information to properties in GitHubPullrequestPoller Mar 26, 2017

noc0lour force-pushed the extent_gh_information branch 4 times, most recently from cd47fc1 to f13056f Compare March 26, 2017 06:45

noc0lour force-pushed the extent_gh_information branch 3 times, most recently from 57d6210 to a28783c Compare March 27, 2017 03:19

tardyp requested changes Mar 27, 2017

View reviewed changes

changes: fix pollAtLaunch and author email parsing in github

5bcc95f

noc0lour force-pushed the extent_gh_information branch from a28783c to 4515672 Compare April 5, 2017 06:24

changes: add additional information to properties in GitHub PR poller

c04c884

noc0lour force-pushed the extent_gh_information branch from 4515672 to fc07f35 Compare April 5, 2017 06:34

changes/hooks: sync GitHubPullrequestPoller and GitHub webhook features

e908d9d

noc0lour force-pushed the extent_gh_information branch from fc07f35 to e908d9d Compare April 5, 2017 06:54

tardyp approved these changes Apr 6, 2017

View reviewed changes

github: replace assertDictContainsSubset with own function

bb728e1

tardyp merged commit 0334b20 into buildbot:master Apr 12, 2017

Conversation

noc0lour commented Mar 25, 2017

Uh oh!

mention-bot commented Mar 25, 2017

Uh oh!

noc0lour commented Mar 25, 2017

Uh oh!

tardyp commented Mar 25, 2017

Uh oh!

noc0lour commented Mar 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tardyp commented Mar 25, 2017

Uh oh!

codecov bot commented Mar 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

noc0lour commented Mar 26, 2017

Uh oh!

seankelly commented Mar 27, 2017

Uh oh!

tardyp Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

tardyp Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

noc0lour Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

tardyp Mar 27, 2017

Choose a reason for hiding this comment

Uh oh!

tardyp commented Mar 27, 2017 via email

Uh oh!

noc0lour commented Mar 29, 2017

Uh oh!

noc0lour commented Apr 6, 2017

Uh oh!

tardyp commented Apr 6, 2017

Uh oh!

tardyp left a comment

Choose a reason for hiding this comment

Uh oh!

tardyp Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

tardyp Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

tardyp Apr 6, 2017

Choose a reason for hiding this comment

Uh oh!

noc0lour commented Apr 6, 2017

Uh oh!

noc0lour commented Apr 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

noc0lour commented Mar 25, 2017 •

edited

Loading

codecov bot commented Mar 26, 2017 •

edited

Loading