Merge pull request buildbot#7616 from mokibit/remove-handleLegacyResult

reporters: Remove _handleLegacyResult() as callback must return a dictionary now

master/buildbot/reporters/gerrit.py

@@ -53,23 +53,6 @@ def makeReviewResult(message, *labels):
     return {"message": message, "labels": dict(labels)}
 
 
-def _handleLegacyResult(result):
-    """
-    make sure the result is backward compatible
-    """
-    if not isinstance(result, dict):
-        warnings.warn(
-            'The Gerrit status callback uses the old way to '
-            'communicate results.  The outcome might be not what is '
-            'expected.'
-        )
-        message, verified, reviewed = result
-        result = makeReviewResult(
-            message, (GERRIT_LABEL_VERIFIED, verified), (GERRIT_LABEL_REVIEWED, reviewed)
-        )
-    return result
-
-
 def _old_add_label(label, value):
     if label == GERRIT_LABEL_VERIFIED:
         return [f"--verified {int(value)}"]
@@ -264,8 +247,6 @@ def get_build_info(build):
             build_info_list, Results[buildset["results"]], master, self.callback_arg
         )
 
-        result = _handleLegacyResult(result)
-
         return {
             "body": result.get("message", None),
             "extra_info": {
@@ -294,8 +275,6 @@ def generate(self, master, reporter, key, message):
 
         result = yield self.callback(build["builder"]["name"], build, self.callback_arg)
 
-        result = _handleLegacyResult(result)
-
         return {
             "body": result.get("message", None),
             "extra_info": {
@@ -326,8 +305,6 @@ def generate(self, master, reporter, key, message):
             build['builder']['name'], build, build['results'], master, self.callback_arg
         )
 
-        result = _handleLegacyResult(result)
-
         return {
             "body": result.get("message", None),
             "extra_info": {

master/buildbot/test/unit/reporters/test_gerrit.py

@@ -118,31 +118,6 @@ def sampleSummaryCBDeferred(buildInfoList, results, master, arg):
     return result
 
 
-def legacyTestReviewCB(builderName, build, result, status, arg):
-    msg = str({'name': builderName, 'result': result})
-    return (msg, 1 if result == SUCCESS else -1, 0)
-
-
-def legacyTestSummaryCB(buildInfoList, results, status, arg):
-    success = False
-    failure = False
-
-    for buildInfo in buildInfoList:
-        if buildInfo['result'] == SUCCESS:  # pylint: disable=simplifiable-if-statement
-            success = True
-        else:
-            failure = True
-
-    if failure:
-        verified = -1
-    elif success:
-        verified = 1
-    else:
-        verified = 0
-
-    return (str(buildInfoList), verified, 0)
-
-
 class TestGerritStatusPush(TestReactorMixin, unittest.TestCase, ReporterTestMixin):
     def setUp(self):
         self.setup_test_reactor()
@@ -210,10 +185,6 @@ def run_fake_summary_build(self, gsp, buildResults, finalResult, resultText, exp
             )
         return str(info)
 
-    # check_summary_build and check_summary_build_legacy differ in two things:
-    #   * the callback used
-    #   * the expected result
-
     @defer.inlineCallbacks
     def check_summary_build_deferred(self, buildResults, finalResult, resultText, verifiedScore):
         with assertProducesWarnings(DeprecatedApiWarning, message_pattern="Use generators instead"):
@@ -242,22 +213,6 @@ def check_summary_build(self, buildResults, finalResult, resultText, verifiedSco
             {GERRIT_LABEL_VERIFIED: verifiedScore},
         )
 
-    @defer.inlineCallbacks
-    def check_summary_build_legacy(self, buildResults, finalResult, resultText, verifiedScore):
-        with assertProducesWarnings(DeprecatedApiWarning, message_pattern="Use generators instead"):
-            gsp = yield self.setupGerritStatusPush(summaryCB=legacyTestSummaryCB)
-
-        msg = yield self.run_fake_summary_build(
-            gsp, buildResults, finalResult, resultText, expWarning=True
-        )
-
-        gsp.send_code_review.assert_called_once_with(
-            self.reporter_test_project,
-            self.reporter_test_revision,
-            msg,
-            {GERRIT_LABEL_VERIFIED: verifiedScore, GERRIT_LABEL_REVIEWED: 0},
-        )
-
     @defer.inlineCallbacks
     def test_gerrit_ssh_cmd(self):
         kwargs = {
@@ -329,33 +284,6 @@ def test_buildsetComplete_mixed_sends_summary_review(self):
         )
         return d
 
-    def test_buildsetComplete_success_sends_summary_review_legacy(self):
-        d = self.check_summary_build_legacy(
-            buildResults=[SUCCESS, SUCCESS],
-            finalResult=SUCCESS,
-            resultText=["succeeded", "succeeded"],
-            verifiedScore=1,
-        )
-        return d
-
-    def test_buildsetComplete_failure_sends_summary_review_legacy(self):
-        d = self.check_summary_build_legacy(
-            buildResults=[FAILURE, FAILURE],
-            finalResult=FAILURE,
-            resultText=["failed", "failed"],
-            verifiedScore=-1,
-        )
-        return d
-
-    def test_buildsetComplete_mixed_sends_summary_review_legacy(self):
-        d = self.check_summary_build_legacy(
-            buildResults=[SUCCESS, FAILURE],
-            finalResult=FAILURE,
-            resultText=["succeeded", "failed"],
-            verifiedScore=-1,
-        )
-        return d
-
     @parameterized.expand([
         ("matched", ["Builder1"], True),
         ("not_matched", ["foo"], False),
@@ -443,42 +371,12 @@ def check_single_build_deferred(self, buildResult, verifiedScore):
         ]
         gsp.send_code_review.assert_has_calls(calls)
 
-    @defer.inlineCallbacks
-    def check_single_build_legacy(self, buildResult, verifiedScore):
-        with assertProducesWarnings(DeprecatedApiWarning, message_pattern="Use generators instead"):
-            gsp = yield self.setupGerritStatusPush(
-                reviewCB=legacyTestReviewCB, startCB=sampleStartCB
-            )
-
-        msg = yield self.run_fake_single_build(gsp, buildResult, expWarning=True)
-        calls = [
-            call(
-                self.reporter_test_project,
-                self.reporter_test_revision,
-                str({'name': self.reporter_test_builder_name}),
-                {GERRIT_LABEL_REVIEWED: 0},
-            ),
-            call(
-                self.reporter_test_project,
-                self.reporter_test_revision,
-                msg,
-                {GERRIT_LABEL_VERIFIED: verifiedScore, GERRIT_LABEL_REVIEWED: 0},
-            ),
-        ]
-        gsp.send_code_review.assert_has_calls(calls)
-
     def test_buildComplete_success_sends_review(self):
         return self.check_single_build(SUCCESS, 1)
 
     def test_buildComplete_failure_sends_review(self):
         return self.check_single_build(FAILURE, -1)
 
-    def test_buildComplete_success_sends_review_legacy(self):
-        return self.check_single_build_legacy(SUCCESS, 1)
-
-    def test_buildComplete_failure_sends_review_legacy(self):
-        return self.check_single_build_legacy(FAILURE, -1)
-
     # same goes for check_single_build and check_single_build_legacy
     @parameterized.expand([
         ("matched", ["Builder0"], True),

master/docs/manual/configuration/reporters/gerrit_status.rst

@@ -17,8 +17,8 @@ GerritStatusPush can send a separate review for each build that completes, or a
    :param reviewCB: (optional) Called each time a build finishes. Build properties are available. Can be a deferred.
    :param reviewArg: (optional) Argument passed to the review callback.
 
-                    :: If :py:func:`reviewCB` callback is specified, it must return a message and optionally labels. If no message is specified, nothing will be sent to Gerrit.
-                    It should return a dictionary:
+                    :: If :py:func:`reviewCB` callback is specified, it must return a dictionary.
+                    If no message is specified in the dictionary, nothing will be sent to Gerrit.
 
                     .. code-block:: python
 
@@ -42,8 +42,8 @@ GerritStatusPush can send a separate review for each build that completes, or a
    :param startCB: (optional) Called each time a build is started. Build properties are available. Can be a deferred.
    :param startArg: (optional) Argument passed to the start callback.
 
-                    If :py:func:`startCB` is specified, it must return a message and optionally labels. If no message is specified, nothing will be sent to Gerrit.
-                    It should return a dictionary:
+                    If :py:func:`startCB` is specified, it must return a dictionary.
+                    If no message is specified in the dictionary, nothing will be sent to Gerrit.
 
                     .. code-block:: python
 
@@ -61,9 +61,9 @@ GerritStatusPush can send a separate review for each build that completes, or a
    :param summaryCB: (optional) Called each time a buildset finishes. Each build in the buildset has properties available. Can be a deferred.
    :param summaryArg: (optional) Argument passed to the summary callback.
 
-                      If :py:func:`summaryCB` callback is specified, it must return a message and optionally labels. If no message is specified, nothing will be sent to Gerrit.
+                      If :py:func:`summaryCB` callback is specified, it must return a dictionary.
+                      If no message is specified in the dictionary, nothing will be sent to Gerrit.
                       The message and labels should be a summary of all the builds within the buildset.
-                      It should return a dictionary:
 
                       .. code-block:: python
 

newsfragments/gerrit-callback-only-dictionary.removal

@@ -0,0 +1 @@
+Gerrit status callback functions now can only return dictionary type.