Merge ~mthaddon/jenkins-launchpad-plugin/+git/jenkins-launchpad-plugin:comment-if-nominate-fails into jenkins-launchpad-plugin:master

[Tom Haddon]

Add a comment as a fallback if nominateReviewer fails in start_jenkins_job

Add a comment as a fallback if nominateReviewer fails in start_jenkins_job.

Per lp#1917331 allow jenkins-launchpad-plugin to work with less permissions to the project in question.

[Paride Legovini (paride)]

The change LGTM, I'd just add an info or warning message printed by launchpadTrigger to inform the CI maintainer about the fallback to a comment because the bot user doesn't have permissions. I can see myself being confused by the fact that some jobs add the commend, and others do not.

[Tom Haddon (mthaddon)]

Have confirmed this works as expected https://code.launchpad.net/~aluria/juju-verify/+git/juju-verify/+merge/399080

[Alvaro Uria (aluria)]

I've also tested it in 3 different MPs and worked as expected.

See one of the tests [1].

1. https://code.launchpad.net/~aluria/juju-verify/+git/juju-verify/+merge/399084

[Paride Legovini (paride)]

LGTM, thanks!

review: Approve

diff --git a/.gitignore b/.gitignore
index cffb0d6..b2c77d6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -8,5 +8,6 @@ docs/_build/*
 /man
 __pycache__
 *.pyc
+*.swp
 build
 dist
diff --git a/jlp/jenkinsutils.py b/jlp/jenkinsutils.py
index 8b94b1b..9c8a839 100644
--- a/jlp/jenkinsutils.py
+++ b/jlp/jenkinsutils.py
@@ -737,18 +737,34 @@ def start_jenkins_job(lp_handle, launchpaduser, jenkins_url, jenkins_job, mp,
         logger.debug('Job is not buildable (probably disabled)')
         return False
 
+    review_type = get_config_option('launchpad_review_type')
+    subject = launchpadutils.get_vote_subject(mp)
+
     try:
         mp.nominateReviewer(
             reviewer=launchpaduser,
-            review_type=get_config_option('launchpad_review_type'))
+            review_type=review_type)
     except Unauthorized:
+        # If we fail to nominate a review it could be because we don't have
+        # the required permissions, but we may still be able to comment on
+        # the merge propsal, so try that before failing.
         try:
-            username = lp_handle.me.name
+            message = ("A CI job is currently in progress. A follow up "
+                       "comment will be added when it completes.")
+            mp.createComment(
+                content=message,
+                review_type=review_type,
+                subject=subject)
+            logger.info("Missing permissions to nominate a review, so added "
+                        "a comment instead.")
         except Unauthorized:
-            username = '<anonymous>'
-        logger.info('You (' + username + ') are not authorized to review '
-                    'this merge proposal. CI process will now be stopped.')
-        return False
+            try:
+                username = lp_handle.me.name
+            except Unauthorized:
+                username = '<anonymous>'
+            logger.error('You (' + username + ') are not authorized to review '
+                         'this merge proposal. CI process will now be stopped.')
+            return False
 
     try:
         is_merge_only = launchpadutils.is_merge_only(mp)
@@ -778,9 +794,10 @@ def start_jenkins_job(lp_handle, launchpaduser, jenkins_url, jenkins_job, mp,
         logger.debug('Starting a job failed')
         message = "ERROR: failed to start a jenkins job"
         mp.createComment(
-            review_type=get_config_option('launchpad_review_type'),
-            vote='Disapprove', subject=launchpadutils.get_vote_subject(mp),
-            content=message)
+            content=message,
+            review_type=review_type,
+            subject=subject,
+            vote='Disapprove')
 
 
 def trigger_ci_build(lp_handle, mps, jenkins_job, jenkins_url):
diff --git a/tests/test_jenkinsutils.py b/tests/test_jenkinsutils.py
index 8c7bd85..e0565d2 100644
--- a/tests/test_jenkinsutils.py
+++ b/tests/test_jenkinsutils.py
@@ -1,4 +1,4 @@
-from unittest.mock import MagicMock, patch
+from unittest.mock import MagicMock, call, patch
 from jlp import jenkinsutils, launchpadutils, _config
 import unittest
 from tests import JenkinsJSONData
@@ -954,6 +954,14 @@ class TestStartJenkinsJob(unittest.TestCase):
                                            'url', 'jenkins_job', mp)
             self.assertEqual(j.build_job.call_count, 1)
             self.assertEqual(mp.nominateReviewer.call_count, 1)
+            expected_call_args_list = [
+                call(reviewer=self.launchpad_user,
+                     review_type='continuous-integration')]
+            self.assertEqual(mp.nominateReviewer.call_args_list,
+                             expected_call_args_list)
+            # We have no failure to nominateReviewer, so there is
+            # no need to add a comment.
+            self.assertEqual(mp.createComment.call_count, 0)
 
     def test_start_jenkins_job_while_diff_is_not_yet_generated(self):
         mp = MagicMock()
@@ -985,10 +993,12 @@ class TestStartJenkinsJob(unittest.TestCase):
                                            'url', 'jenkins_job', mp)
             self.assertEqual(mp.createComment.call_count, 1)
 
-    def test_start_jenkins_job_and_raise_unauthorized(self):
+    def test_start_jenkins_job_nominate_unauthorized(self):
         mp = MagicMock()
         mp.web_link = 'http://my-example-url.com'
         mp.nominateReviewer.side_effect = Unauthorized("unauthorized", '')
+        mp.source_branch.display_name = "source_branch"
+        mp.target_branch.display_name = "target_branch"
         j = MagicMock()
         j.get_job_info = MagicMock(return_value={'buildable': True})
         jenkinsInstance = MagicMock()
@@ -997,7 +1007,35 @@ class TestStartJenkinsJob(unittest.TestCase):
             ret = jenkinsutils.start_jenkins_job(self.lp_handle,
                                                  self.launchpad_user, 'url',
                                                  'jenkins_job', mp)
-            self.assertEqual(mp.createComment.call_count, 0)
+            self.assertEqual(mp.nominateReviewer.call_count, 1)
+            # nominateReviewer is raising an exception, but createComment
+            # can complete without problems.
+            self.assertEqual(mp.createComment.call_count, 1)
+            expected_content = ("A CI job is currently in progress. A follow "
+                                "up comment will be added when it completes.")
+            expected_subject = "Re: [Merge] source_branch into target_branch"
+            expected_call_args_list = [
+                call(content=expected_content,
+                     review_type='continuous-integration',
+                     subject=expected_subject)]
+            self.assertEqual(mp.createComment.call_args_list,
+                             expected_call_args_list)
+
+    def test_start_jenkins_job_nominate_and_comment_unauthorized(self):
+        mp = MagicMock()
+        mp.web_link = 'http://my-example-url.com'
+        mp.nominateReviewer.side_effect = Unauthorized("unauthorized", '')
+        mp.createComment.side_effect = Unauthorized("unauthorized", '')
+        j = MagicMock()
+        j.get_job_info = MagicMock(return_value={'buildable': True})
+        jenkinsInstance = MagicMock()
+        jenkinsInstance.Jenkins = MagicMock(return_value=j)
+        with patch('jlp.jenkinsutils.jenkins', new=jenkinsInstance):
+            ret = jenkinsutils.start_jenkins_job(self.lp_handle,
+                                                 self.launchpad_user, 'url',
+                                                 'jenkins_job', mp)
+            self.assertEqual(mp.nominateReviewer.call_count, 1)
+            self.assertEqual(mp.createComment.call_count, 1)
             self.assertEqual(ret, False)
 
     def test_start_nonbuildable_jenkins_job(self):