Control who gets notified by GerritStatusPush #2411

dinatale2 · 2016-09-23T17:48:19Z

Allow the GerritStatusPush object to accept who to notify as an argument. The notify argument is a string
which accepts values found in the gerrit documentation for the --notify flag. The argument is set to None by
default to maintain current functionality.

Signed-off-by: Giuseppe Di Natale dinatale2@llnl.gov

mention-bot · 2016-09-23T17:48:21Z

@dinatale2, thanks for your PR! By analyzing the annotation information on this pull request, we identified @tardyp, @adeason and @sa2ajj to be potential reviewers

codecov-io · 2016-09-23T18:36:05Z

Current coverage is 86.75% (diff: 100%)

Merging #2411 into master will increase coverage by <.01%

@@             master      #2411   diff @@
==========================================
  Files           298        298          
  Lines         30940      30944     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          26842      26846     +4   
  Misses         4098       4098          
  Partials          0          0

Powered by Codecov. Last update f7be7bc...3945799

tardyp · 2016-09-23T19:58:42Z

Hi we would appreciate some unit tests to test this feature, so that it does not reduce the coverage

sa2ajj · 2016-09-24T00:41:27Z

master/buildbot/reporters/gerrit.py

@@ -175,6 +176,7 @@ def reconfigService(self, server, username, reviewCB=DEFAULT_REVIEW,
        self.summaryCB = summaryCB
        self.summaryArg = summaryArg
        self.builders = builders
+        self.gerrit_notify = notify


while legacy code extensively uses public members, the new code is expected to be aware whether it's a part of the class' API or not. I'd suggest to use _gerrit_notify

sa2ajj · 2016-09-24T00:42:20Z

master/buildbot/reporters/gerrit.py

@@ -397,6 +399,10 @@ def sendCodeReview(self, project, revision, result):
            return

        command = self._gerritCmd("review", "--project %s" % (project,))
+
+        if self.gerrit_notify is not None:
+            command.extend(["--notify %s" % str(self.gerrit_notify)])


why do you use .extend for a single item? use .append

please use ' not "

sa2ajj · 2016-09-24T00:42:40Z

master/buildbot/test/unit/test_reporter_gerrit.py

@@ -142,7 +142,8 @@ def setUp(self):
    def setupGerritStatusPushSimple(self, *args, **kwargs):
        serv = kwargs.pop("server", "serv")
        username = kwargs.pop("username", "user")
-        gsp = GerritStatusPush(serv, username, *args, **kwargs)
+        notify = kwargs.pop("notify", None)


ditto for "

sa2ajj · 2016-09-24T00:44:00Z

master/buildbot/test/unit/test_reporter_gerrit.py

+
+        # now test the notify argument
+        gsp.gerrit_notify = 'OWNER'
+        gsp.processVersion("2.6", lambda: None)


please make sure that all strings that do not include ' use "

I'm not a python pro by any means. Do you mind clarifying when to use ' instead of "? For one of the other comments above (in the .extend comment), you suggested I use ' instead of " is why I ask.

Well, per String Quotes in PEP-8, both " and ' are OK, however it's good to stick to some particular way.

So far, we've been using " for docstrings only """ and ' for all other string literals (some legacy code does not follow this, I must admit).

sa2ajj · 2016-09-29T16:20:19Z

master/buildbot/test/unit/test_reporter_gerrit.py

+
+        # now test the notify argument, even though _gerrit_notify
+        # is private, work around that
+        gsp._gerrit_notify = "OWNER"


please make sure that you use single quotes everywhere (even in tests).

sa2ajj · 2016-09-29T16:20:53Z

master/buildbot/test/unit/test_reporter_gerrit.py

+            ['ssh', 'user@serv', '-p', '29418', 'gerrit', 'review',
+             '--project project', '--notify OWNER', "--message 'bla'", '--label Verified=1', 'revision'])
+
+        # <=2.5 uses other syntax


2.5 of what?

dinatale2 · 2016-10-10T15:04:48Z

I've updated this replacing the " with ' and I've introduced a comment about the different syntax with gerrit versions 2.5 and below. I'm not sure why the bb test failed as well, it appears to have been cancelled.

tardyp · 2016-10-10T17:02:08Z

looks good. There is stil a need for documentation update, and release notes

tardyp

Needs doc update and relnotes update.
thanks!

dinatale2 · 2016-10-12T15:49:21Z

docs and release notes have been updated. Let me know if I need to make any additional changes.

tardyp · 2016-10-12T17:03:01Z

master/docs/manual/cfg-reporters.rst

@@ -738,6 +738,7 @@ GerritStatusPush can send a separate review for each build that completes, or a
   :param builders: (optional) list of builders to send results for.
                    This method allows to filter results for a specific set of builder.
                    By default, or if builders is None, then no filtering is performed.
+   :param notify: (optional) control who gets notified by Gerrit once the status is posted.


I think this would deserve to document which values are possible, probably point on the gerrit documentation, explaining that this correspond to the --notify values.

Allow the GerritStatusPush object to accept who to notify as an argument. The notify argument is a string which accepts values found in the gerrit documentation for the --notify flag. The argument is set to None by default to maintain current functionality. Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>

sa2ajj

LGTM

dinatale2 force-pushed the upstream/GerritStatusPush branch from a702e1e to 3f23f12 Compare September 23, 2016 18:17

dinatale2 force-pushed the upstream/GerritStatusPush branch 3 times, most recently from b021f07 to edf3837 Compare September 23, 2016 23:21

sa2ajj requested changes Sep 24, 2016

View reviewed changes

sa2ajj added the needs work label Sep 24, 2016

dinatale2 force-pushed the upstream/GerritStatusPush branch 3 times, most recently from 1d12236 to ab229f8 Compare September 24, 2016 02:36

sa2ajj requested changes Sep 29, 2016

View reviewed changes

dinatale2 force-pushed the upstream/GerritStatusPush branch from ab229f8 to e00dd9b Compare September 29, 2016 17:19

tardyp added needs docs needs relnotes and removed needs work labels Oct 10, 2016

sa2ajj approved these changes Oct 12, 2016

View reviewed changes

tardyp requested changes Oct 12, 2016

View reviewed changes

dinatale2 force-pushed the upstream/GerritStatusPush branch from e00dd9b to 67236ac Compare October 12, 2016 15:48

tardyp reviewed Oct 12, 2016

View reviewed changes

dinatale2 force-pushed the upstream/GerritStatusPush branch from 67236ac to 3945799 Compare October 12, 2016 17:32

sa2ajj approved these changes Oct 16, 2016

View reviewed changes

sa2ajj added merge me and removed needs docs needs relnotes labels Oct 16, 2016

tardyp merged commit 729c1be into buildbot:master Oct 16, 2016

dinatale2 deleted the upstream/GerritStatusPush branch October 17, 2016 14:50

dinatale2 mentioned this pull request Oct 17, 2016

Control who gets notified by GerritStatusPush opensfs/buildbot#14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control who gets notified by GerritStatusPush #2411

Control who gets notified by GerritStatusPush #2411

dinatale2 commented Sep 23, 2016 •

edited by sa2ajj

mention-bot commented Sep 23, 2016

codecov-io commented Sep 23, 2016 •

edited

tardyp commented Sep 23, 2016

sa2ajj Sep 24, 2016

sa2ajj Sep 24, 2016

sa2ajj Sep 24, 2016

sa2ajj Sep 24, 2016

dinatale2 Sep 24, 2016 •

edited

sa2ajj Sep 24, 2016

sa2ajj Sep 29, 2016

sa2ajj Sep 29, 2016

dinatale2 commented Oct 10, 2016

tardyp commented Oct 10, 2016

tardyp left a comment

dinatale2 commented Oct 12, 2016

tardyp Oct 12, 2016

dinatale2 Oct 12, 2016

sa2ajj left a comment

Control who gets notified by GerritStatusPush #2411

Control who gets notified by GerritStatusPush #2411

Conversation

dinatale2 commented Sep 23, 2016 • edited by sa2ajj

mention-bot commented Sep 23, 2016

codecov-io commented Sep 23, 2016 • edited

Current coverage is 86.75% (diff: 100%)

tardyp commented Sep 23, 2016

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

dinatale2 Sep 24, 2016 • 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

dinatale2 commented Oct 10, 2016

tardyp commented Oct 10, 2016

tardyp left a comment

Choose a reason for hiding this comment

dinatale2 commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sa2ajj left a comment

Choose a reason for hiding this comment

dinatale2 commented Sep 23, 2016 •

edited by sa2ajj

codecov-io commented Sep 23, 2016 •

edited

dinatale2 Sep 24, 2016 •

edited