#3957 Implement a button to rerun CI tests on a pull request. Fixes #3552
Merged 5 years ago by pingou. Opened 5 years ago by bkabrda.
bkabrda/pagure rerun-ci-button  into  master

file modified
+16 -5
@@ -1219,14 +1219,25 @@ 

  ~~~~~~~~~~

  

  A run of pagure-ci can be manually triggered if some key sentences are added

- as comment to a pull-request. This allows to re-run a test that failed due

- to some network outage or other unexpected issues unrelated to the test

- suite.

+ as comment to a pull-request, either manually or via the "Rerun CI" dropdown.

+ This allows to re-run a test that failed due to some network outage or other

+ unexpected issues unrelated to the test suite.

  

  This configuration key allows to define all the sentences that can be used

- to trigger this pagure-ci run.

+ to trigger this pagure-ci run. The format is following: ``{"<sentence>":

+ {"name": "<name of the CI>", "description": "<short description>"}}``

  

- Defaults to: ``['pretty please pagure-ci rebuild']``

+ Sentences which have ``None`` as value won't show up in the "Rerun CI"

+ dropdown. Additionally, it's possible to add a ``requires_project_hook_attr``

+ key to the dict with data about a sentence. For example, having

+ ``"requires_project_hook_attr": ("ci_hook", "active_pr", True)`` would make

+ the "Rerun CI" dropdown have a button for this specific CI only if the

+ project has ``ci_hook`` activated and its ``active_pr`` value is ``True``.

+ 

+ In versions before 5.2, this was a list containing just the sentences.

+ 

+ Defaults to: ``{"pretty please pagure-ci rebuild": {"name": "Default CI",

+ "description": "Rerun default CI"}}``

  

  .. note:: The sentences defined in this configuration key should be lower

            case only!

file modified
+7 -1
@@ -376,7 +376,13 @@ 

  # unless the user has direct access to it.

  EXCLUDE_GROUP_INDEX = []

  

- TRIGGER_CI = ["pretty please pagure-ci rebuild"]

+ TRIGGER_CI = {

+     "pretty please pagure-ci rebuild": {

+         "name": "Default CI",

+         "description": "Rerun default CI",

+         "requires_project_hook_attr": ("ci_hook", "active_pr", True),

+     },

+ }

  

  FLAG_STATUSES_LABELS = {

      "success": "badge-success",

file modified
+21
@@ -862,3 +862,24 @@ 

          [wtforms.validators.optional()],

          false_values=FALSE_VALUES,

      )

+ 

+ 

+ class TriggerCIPRForm(PagureForm):

+     def __init__(self, *args, **kwargs):

+         # we need to instantiate dynamically because the configuration

+         # values may change during tests and we want to always respect

+         # the currently set value

+         super(TriggerCIPRForm, self).__init__(*args, **kwargs)

+         choices = []

+         trigger_ci = pagure_config["TRIGGER_CI"]

+         if isinstance(trigger_ci, dict):

+             # make sure to preserver compatibility with older configs

+             # which had TRIGGER_CI as a list

+             for comment, meta in trigger_ci.items():

+                 if meta is not None:

+                     choices.append((comment, comment))

+         self.comment.choices = choices

+ 

+     comment = wtforms.SelectField(

+         "comment", [wtforms.validators.Required()], choices=[]

+     )

file modified
+4 -1
@@ -1377,6 +1377,7 @@ 

  

      # Send notification to the CI server, if the comment added was a

      # notification and the PR is still open and project is not private

+     ci_triggered = False

      if (

          notification

          and request.status == "Open"
@@ -1391,6 +1392,7 @@ 

              branch=request.branch_from,

              ci_type=request.project.ci_hook.ci_type,

          )

+         ci_triggered = True

  

      pagure.lib.notify.log(

          request.project,
@@ -1402,7 +1404,8 @@ 

      )

  

      if (

-         trigger_ci

+         not ci_triggered

+         and trigger_ci

          and comment.strip().lower() in trigger_ci

          and pagure_config.get("PAGURE_CI_SERVICES")

          and request.project.ci_hook

@@ -170,36 +170,60 @@ 

          {% endif %}

          {% if pull_request.status == 'Open' %}

          <div class="dropdown float-right ml-1">

-         <a href="#" id="merge_dropdown_btn"

-         class="btn btn-outline-secondary btn-sm disabled dropdown-toggle" data-toggle="dropdown">

-           <span class="fa fa-circle-o-notch fa-spin fa-fw"></span> <span id="merge-status-text">Merge</span>

+         <span class="dropdown dropdown-btn">

+           <a href="#" id="merge_dropdown_btn"

+           class="btn btn-outline-secondary btn-sm disabled dropdown-toggle" data-toggle="dropdown">

+             <span class="fa fa-circle-o-notch fa-spin fa-fw"></span> <span id="merge-status-text">Merge</span>

            </a>

- 

-         <div id="merge-alert" class="text-xs-center dropdown-menu dropdown-menu-right p-0" style="min-width:400px">

+           <div id="merge-alert" class="text-xs-center dropdown-menu dropdown-menu-right p-0" style="min-width:400px">

               <div class="alert text-center mb-0">

-               {% if pull_request.status == 'Open' and g.repo_committer %}

+                 {% if pull_request.status == 'Open' and g.repo_committer %}

+                 <small id="merge-alert-message"></small>

+                 <form action="{{ url_for('ui_ns.merge_request_pull',

+                         repo=repo.name,

+                         username=username,

+                         namespace=repo.namespace,

+                         requestid=requestid)

+                   }}" method="POST">

+                   {{ mergeform.csrf_token }}

+                   <button id="merge_btn" type="submit"

+                     onclick="return confirm('Confirm merging this pull-request');"

+                     class="btn btn-block my-2">Merge</button>

+                   {% if can_delete_branch %}

+                   <div class="small">

+                   {{ mergeform.delete_branch }} {{ mergeform.delete_branch.label }}

+                   </div>

+                   {% endif %}

+                 </form>

+               {% else %}

                <small id="merge-alert-message"></small>

-               <form action="{{ url_for('ui_ns.merge_request_pull',

-                       repo=repo.name,

-                       username=username,

-                       namespace=repo.namespace,

-                       requestid=requestid)

-                 }}" method="POST">

-                 {{ mergeform.csrf_token }}

-                 <button id="merge_btn" type="submit"

-                   onclick="return confirm('Confirm merging this pull-request');"

-                   class="btn btn-block my-2">Merge</button>

-                 {% if can_delete_branch %}

-                 <div class="small">

-                 {{ mergeform.delete_branch }} {{ mergeform.delete_branch.label }}

-                 </div>

-                 {% endif %}

-               </form>

-             {% else %}

-             <small id="merge-alert-message"></small>

-             {% endif %}

+               {% endif %}

+             </div>

            </div>

-         </div>

+         </span>

+         {% if g.authenticated and trigger_ci %}

+         <span class="dropdown dropdown-btn">

+           <a href="#" id="ci_dropdown_btn"

+           class="btn btn-outline-primary btn-sm btn-info dropdown-toggle" data-toggle="dropdown">

+             Rerun CI

+           </a>

+           <div class="text-xs-center dropdown-menu dropdown-menu-right p-0">

+             <form action="{{ url_for('ui_ns.ci_trigger_request_pull',

+                     repo=repo.name,

+                     username=username,

+                     namespace=repo.namespace,

+                     requestid=requestid)

+               }}" method="POST" id="ci_pr_trigger_form">

+               {{ trigger_ci_pr_form.csrf_token }}

+               {{ trigger_ci_pr_form.comment(id="ci_pr_comment", hidden=True) }}

+                 {% for comment, meta in trigger_ci|dictsort %}

+                   <a class="dropdown-item" href="#" title="{{ meta["description"] }}"

+                   onclick="$('#ci_pr_comment').val('{{ comment }}'); $('#ci_pr_trigger_form').submit()">{{ meta["name"] }}</a>

+                 {% endfor %}

+             </form>

+           </div>

+         </span>

+         {% endif %}

        </div>

        {% endif %}

        {% if pull_request.status == 'Closed' and g.authenticated and

file modified
+95 -1
@@ -30,6 +30,7 @@ 

  import pagure.doc_utils

  import pagure.exceptions

  import pagure.lib.git

+ import pagure.lib.plugins

  import pagure.lib.query

  import pagure.lib.tasks

  import pagure.forms
@@ -308,6 +309,24 @@ 

          diff.find_similar()

  

      form = pagure.forms.MergePRForm()

+     trigger_ci_pr_form = pagure.forms.TriggerCIPRForm()

+ 

+     # we need to leave out all members of trigger_ci_conf that have

+     # "meta" set to False or meta["requires_project_hook_attr"] condition

+     # defined and it's not met

+     trigger_ci_conf = pagure_config["TRIGGER_CI"]

+     if not isinstance(trigger_ci_conf, dict):

+         trigger_ci_conf = {}

+     trigger_ci = {}

+     # make sure all the backrefs are set properly on repo

+     pagure.lib.plugins.get_enabled_plugins(repo)

+     for comment, meta in trigger_ci_conf.items():

+         if not meta:

+             continue

+         cond = meta.get("requires_project_hook_attr", ())

+         if cond and not pagure.utils.project_has_hook_attr_value(repo, *cond):

+             continue

+         trigger_ci[comment] = meta

  

      can_delete_branch = (

          pagure_config.get("ALLOW_DELETE_BRANCH", True)
@@ -328,6 +347,8 @@ 

          subscribers=pagure.lib.query.get_watch_list(flask.g.session, request),

          tag_list=pagure.lib.query.get_tags_of_project(flask.g.session, repo),

          can_delete_branch=can_delete_branch,

+         trigger_ci=trigger_ci,

+         trigger_ci_pr_form=trigger_ci_pr_form,

      )

  

  
@@ -619,6 +640,9 @@ 

          comment = form.comment.data

  

          try:

+             trigger_ci = pagure_config["TRIGGER_CI"]

+             if isinstance(trigger_ci, dict):

+                 trigger_ci = list(trigger_ci.keys())

              message = pagure.lib.query.add_pull_request_comment(

                  flask.g.session,

                  request=request,
@@ -628,7 +652,7 @@ 

                  row=row,

                  comment=comment,

                  user=flask.g.fas_user.username,

-                 trigger_ci=pagure_config["TRIGGER_CI"],

+                 trigger_ci=trigger_ci,

              )

              flask.g.session.commit()

              if not is_js:
@@ -931,6 +955,76 @@ 

      )

  

  

+ @UI_NS.route(

+     "/<repo>/pull-request/<int:requestid>/trigger-ci",

+     methods=["POST"]

+ )

+ @UI_NS.route(

+     "/<namespace>/<repo>/pull-request/<int:requestid>/trigger-ci",

+     methods=["POST"]

+ )

+ @UI_NS.route(

+     "/fork/<username>/<repo>/pull-request/<int:requestid>/trigger-ci",

+     methods=["POST"],

+ )

+ @UI_NS.route(

+     ("/fork/<username>/<namespace>/<repo>/pull-request/"

+      "<int:requestid>/trigger-ci"),

+     methods=["POST"],

+ )

+ @login_required

+ def ci_trigger_request_pull(repo, requestid, username=None, namespace=None):

+     """ Trigger CI testing for a PR.

+     """

+     form = pagure.forms.TriggerCIPRForm()

+     if not form.validate_on_submit():

+         flask.flash("Invalid input submitted", "error")

+         return flask.redirect(

+             flask.url_for(

+                 "ui_ns.request_pull",

+                 repo=repo,

+                 requestid=requestid,

+                 username=username,

+                 namespace=namespace,

+             )

+         )

+ 

+     repo_obj = flask.g.repo

+     request = pagure.lib.query.search_pull_requests(

+         flask.g.session, project_id=repo_obj.id, requestid=requestid

+     )

+ 

+     if not request:

+         flask.abort(404, "Pull-request not found")

+ 

+     trigger_ci = pagure_config["TRIGGER_CI"]

+     if isinstance(trigger_ci, dict):

+         trigger_ci = list(trigger_ci.keys())

+     pagure.lib.query.add_pull_request_comment(

+         flask.g.session,

+         request,

+         commit=None,

+         tree_id=None,

+         filename=None,

+         row=None,

+         comment=form.comment.data,

+         user=flask.g.fas_user.username,

+         notify=True,

+         notification=True,

+         trigger_ci=trigger_ci,

+     )

+ 

+     return flask.redirect(

+         flask.url_for(

+             "ui_ns.request_pull",

+             repo=repo,

+             username=username,

+             namespace=namespace,

+             requestid=requestid,

+         )

+     )

+ 

+ 

  @UI_NS.route("/<repo>/pull-request/<int:requestid>/merge", methods=["POST"])

  @UI_NS.route(

      "/<namespace>/<repo>/pull-request/<int:requestid>/merge", methods=["POST"]

file modified
+24
@@ -664,3 +664,27 @@ 

          if key.id == keyid:

              return key

      return None

+ 

+ 

+ def project_has_hook_attr_value(project, hook, attr, value):

+     """ Finds out if project's hook has attribute of given value.

+ 

+     :arg project: The project to inspect

+     :type project: pagure.lib.model.Project

+     :arg hook: Name of the hook to inspect

+     :type hook: str

+     :arg attr: Name of hook attribute to inspect

+     :type attr: str

+     :arg value: Value to compare project's hook attribute value with

+     :type value: object

+     :return: True if project's hook attribute value is equal with given

+         value, False otherwise

+     """

+     retval = False

+     hook_obj = getattr(project, hook, None)

+     if hook_obj is not None:

+         attr_obj = getattr(hook_obj, attr, None)

+         if attr_obj == value:

+             retval = True

+ 

+     return retval

@@ -378,6 +378,156 @@ 

          self.assertNotEqual(stop_commit, request.commit_stop)

  

      @patch('pagure.lib.notify.send_email')

+     def test_request_pull_ci_dropdown(self, send_email):

+         """ Test presence of the "Rerun CI" dropdown with various settings. """

+         send_email.return_value = True

+ 

+         tests.create_projects(self.session)

+         tests.create_projects_git(

+             os.path.join(self.path, 'requests'), bare=True)

+         self.set_up_git_repo(new_project=None, branch_from='feature')

+ 

+         user = tests.FakeUser()

+         user.username = 'pingou'

+         with tests.user_set(self.app.application, user):

+             # old-style TRIGGER_CI list - test backwards compatibility

+             with patch.dict('pagure.config.config',

+                             {'TRIGGER_CI': ['old-style-trigger-ci']}):

+                 output = self.app.get('/test/pull-request/1')

+                 self.assertEqual(output.status_code, 200)

+                 output_text = output.get_data(as_text=True)

+                 self.assertNotIn('Rerun CI', output_text)

+ 

+             # new-style TRIGGER_CI, but no button to show

+             with patch.dict('pagure.config.config',

+                             {'TRIGGER_CI': {'no-button': None}}):

+                 output = self.app.get('/test/pull-request/1')

+                 self.assertEqual(output.status_code, 200)

+                 output_text = output.get_data(as_text=True)

+                 self.assertNotIn('Rerun CI', output_text)

+ 

+             trigger_ci = {

+                 'foobar-ci': {

+                     'name': 'foobar-ci-name',

+                     'description': 'barfoo',

+                 },

+                 'spam-ci': {

+                     'name': 'spam-ci-name',

+                     'description': 'with beans and eggs',

+                 },

+                 'no-button-for-me-ci': None,

+             }

+             # new-style TRIGGER_CI, several buttons to show

+             with patch.dict('pagure.config.config',

+                             {'TRIGGER_CI': trigger_ci}):

+                 output = self.app.get('/test/pull-request/1')

+                 self.assertEqual(output.status_code, 200)

+                 output_text = output.get_data(as_text=True)

+                 self.assertIn('Rerun CI', output_text)

+                 self.assertIn('foobar-ci-name', output_text)

+                 self.assertIn('spam-ci-name', output_text)

+                 self.assertNotIn('no-button-for-me-ci', output_text)

+ 

+             trigger_ci = {

+                 'foobar-ci': {

+                     'name': 'foobar-ci-name',

+                     'description': 'barfoo',

+                     'requires_project_hook_attr': ('ci_hook', 'active_pr', True),

+                 },

+             }

+             # new-style TRIGGER_CI with requires_project_hook_attr that is

+             # not fulfilled by the project

+             with patch.dict('pagure.config.config',

+                             {'TRIGGER_CI': trigger_ci}):

+                 output = self.app.get('/test/pull-request/1')

+                 self.assertEqual(output.status_code, 200)

+                 output_text = output.get_data(as_text=True)

+                 self.assertNotIn('Rerun CI', output_text)

+             # now activate the hook and try again

+             data = {

+                 'active_pr': 'y',

+                 'ci_url': 'https://jenkins.fedoraproject.org',

+                 'ci_job': 'ci_job',

+                 'ci_type': 'jenkins',

+                 'csrf_token': self.get_csrf()

+             }

+             output = self.app.post('/test/settings/Pagure CI', data=data,

+                                    follow_redirects=True)

+             self.assertEqual(output.status_code, 200)

+             with patch.dict('pagure.config.config',

+                             {'TRIGGER_CI': trigger_ci}):

+                 output = self.app.get('/test/pull-request/1')

+                 self.assertEqual(output.status_code, 200)

+                 output_text = output.get_data(as_text=True)

+                 self.assertIn('Rerun CI', output_text)

+                 self.assertIn('foobar-ci-name', output_text)

+ 

+         # shouldn't show up if user is not logged in

+         with patch.dict('pagure.config.config',

+                         {'TRIGGER_CI': trigger_ci}):

+             output = self.app.get('/test/pull-request/1')

+             self.assertEqual(output.status_code, 200)

+             output_text = output.get_data(as_text=True)

+             self.assertNotIn('Rerun CI', output_text)

+ 

+     @patch('pagure.lib.notify.send_email')

+     @patch.dict('pagure.config.config',

+                 {'TRIGGER_CI': {'CI1': {'name': 'CI1', 'description': 'CI1!'}}})

+     def test_request_pull_ci_rerun(self, send_email):

+         """ Test rerunning CI using button from the "Rerun CI" dropdown. """

+         send_email.return_value = True

+ 

+         tests.create_projects(self.session)

+         tests.create_projects_git(

+             os.path.join(self.path, 'requests'), bare=True)

+         self.set_up_git_repo(new_project=None, branch_from='feature')

+         user = tests.FakeUser()

+         user.username = 'pingou'

+         project = pagure.lib.query.get_authorized_project(self.session, 'test')

+         request = project.requests[0]

+ 

+         with tests.user_set(self.app.application, user):

+             # no csrf token

+             output = self.app.get('/test/pull-request/1')

+             self.assertEqual(output.status_code, 200)

+             output = self.app.post(

+                 '/test/pull-request/1/trigger-ci', follow_redirects=True)

+             self.assertEqual(output.status_code, 200)

+             self.assertIn('Invalid input', output.get_data(as_text=True))

+ 

+             # no such PR

+             output = self.app.get('/test/pull-request/1')

+             self.assertEqual(output.status_code, 200)

+             output = self.app.post(

+                 '/test/pull-request/2/trigger-ci', follow_redirects=True)

+             self.assertEqual(output.status_code, 404)

+ 

+             # wrong comment

+             output = self.app.get('/test/pull-request/1')

+             self.assertEqual(output.status_code, 200)

+             csrf_token = self.get_csrf(output=output)

+             data = {'csrf_token': csrf_token, 'comment': 'this doesnt exist'}

+             output = self.app.post(

+                 '/test/pull-request/1/trigger-ci', data=data, follow_redirects=True)

+             self.assertEqual(output.status_code, 200)

+             self.assertIn('Invalid input', output.get_data(as_text=True))

+ 

+             # everything ok

+             output = self.app.get('/test/pull-request/1')

+             self.assertEqual(output.status_code, 200)

+             csrf_token = self.get_csrf(output=output)

+             data = {'csrf_token': csrf_token, 'comment': 'CI1'}

+             output = self.app.post(

+                 '/test/pull-request/1/trigger-ci', data=data, follow_redirects=True)

+             output_text = output.get_data(as_text=True)

+             self.assertEqual(output.status_code, 200)

+             self.assertIn('<p>CI1</p>', output_text)

+             comment = request.comments[0]

+             self.assertTrue(comment.notification)

+             self.assertEqual(comment.comment, 'CI1')

+ 

+ 

+     @patch('pagure.lib.notify.send_email')

      def test_merge_request_pull_FF(self, send_email):

          """ Test the merge_request_pull endpoint with a FF PR. """

          send_email.return_value = True

I could theoretically put the config for this feature directly into TRIGGER_CI, but that would require making it a dict and so it wouldn't preserve backwards compatibility.

I'll implement tests once I get a thumbsup on the general approach.

The approach looks okay to me, but I'd defer to @pingou and @ryanlerch...

An idea for preserving backwards compatibility while utilizing TRIGGER_CI - if TRIGGER_CI is a list, assume there's no way to rerun any CIs. If it's a dict, utilize all of its keys that don't have None as a value for buttons that retrigger CIs.

Additionally, I think it might make sense to include more data, e.g. a text for tooltip that would pop up on mouse-over to explain what rerunning a specific CI means.

Does these make sense?

An idea for preserving backwards compatibility while utilizing TRIGGER_CI - if TRIGGER_CI is a list, assume there's no way to rerun any CIs. If it's a dict, utilize all of its keys that don't have None as a value for buttons that retrigger CIs.

How about:
- If it's a list, assume pagure-ci
- If it's a dict, use the new behavior
Would that not work?

Additionally, I think it might make sense to include more data, e.g. a text for tooltip that would pop up on mouse-over to explain what rerunning a specific CI means.

+1, since it is a dict we can embed more info in it

Does these make sense?

Does to me! :)

rebased onto b8f7b5f0ca952ec4f54759ec0378d63d82025d9f

5 years ago

rebased onto 4ace0241af724ec2d9b24874148f4a66f74bca73

5 years ago

Improved, rebased and tests added. Ready for review @pingou :)

Hm, this is going to show the re-run button on all projects while on pagure.io this would have an effect only on the project that have configured pagure-ci.
I don't see a way around this other than adding a global configuration key

I'm not that familiar with pagure.io and pagure-ci. Is there a way to tell if it is enabled for a project?

Ok, so IIUC we'd need to figure out if the pagure_ci hook is enabled for the project, right? I'll try to think of a sensible way of doing this.

Ok, so IIUC we'd need to figure out if the pagure_ci hook is enabled for the project, right?

Yes

I'll try to think of a sensible way of doing this.

Check how we check any other hooks, should be the same approach. It might be simpler to check this in the controller than in the template fwiw

Yeah, I'm just trying to figure out whether it'd be possible to do it generally, e.g. adding something to the config value like "requires_hook": "PagureCi". I'll try doing something along those lines.

rebased onto e500d1dab8cdf1301dbca540eb0c1fc715a221d5

5 years ago

rebased onto 27440c6c648b709135a7477cf0f74d40a7cb5313

5 years ago

@pingou implemented and ready for re-review.

@bkabrda could you provide some gif or animation to show what this looks like?

@ngompa here you go: https://bkabrda.fedorapeople.org/rerun-ci.webm

I also just realized that this shouldn't show up for users who aren't logged in, I'll fix that.

rebased onto c600dc15ae903a58364e51440745d08266261aff

5 years ago

Fixed, the button will now only show for users that are logged in.

I have on small change for this to work fully:

diff --git a/ pagure/lib/query.py b/ pagure/lib/query.py
index b4ef6aab..b08e2044 100644
--- a/ pagure/lib/query.py      
+++ b/ pagure/lib/query.py      
@@ -1369,6 +1369,7 @@ def add_pull_request_comment(

     # Send notification to the CI server, if the comment added was a
     # notification and the PR is still open and project is not private
+    ci_triggered = False
     if (
         notification
         and request.status == "Open"
@@ -1383,6 +1384,7 @@ def add_pull_request_comment(
             branch=request.branch_from,
             ci_type=request.project.ci_hook.ci_type,
         )
+        ci_triggered = True

     pagure.lib.notify.log(
         request.project,
@@ -1394,7 +1396,8 @@ def add_pull_request_comment(
     )

     if (
-        trigger_ci
+        not ci_triggered
+        and trigger_ci
         and comment.strip().lower() in trigger_ci
         and pagure_config.get("PAGURE_CI_SERVICES")
         and request.project.ci_hook

rebased onto 0915d04

5 years ago

Added and rebased on top of #3970.

rebased onto a80d7c4

5 years ago

Pull-Request has been merged by pingou

5 years ago