Add manual trigger run and retry for CircleCI integration.

Review Request #11208 — Created Oct. 1, 2020 and submitted

MarcusBoay
rbintegrations
master
rbintegrations

Add manual trigger UI for CircleCI integration.

This change adds the ability for Integrations to run CircleCI manually.
A checkbox is added to the CircleCI form that indicates if CircleCI should only
run manually.

Watch the demo video here

Based on work by David Trowbridge: /r/11130

This patch depends on /r/11190, /r/11198 and /r/11204

Manual testing by adding a private GitHub repository with CircleCI integration,
checking and unchecking the "manually run" form checkbox to ensure that no
automated runs and done when a review request is made, and clicking the "Run"
button on the review request page.

Verified the disabled/enabled run manually field by testing on Review Board
3.0.18, 3.0.19 and 4.0.x.

Verified retry functionality when state is in ERROR and TIMEOUT states.

Unit tests have been written to verify that when the configuration is set,
the CircleCI request is not made. Another test to verify that a build is
run when the signal is emitted.

Summary
Add manual trigger for CircleCI integration.
Description From Last Updated

W293 blank line contains whitespace

reviewbotreviewbot

You'll need to add a load method override to this form that conditionally disables the manual run field depending on ...

daviddavid

Can we make the label "Run builds manually"?

daviddavid

Why do we need to check if we should build again? It seems like if we shouldn't build the first ...

daviddavid

Can we add a blank line between these two?

daviddavid

This comment doesn't add anything useful (the code is pretty clear that it's initializing variables)

daviddavid

Grammar here is a little off. It sounds like it should probably have a plural "requests": "Don't build any review ...

daviddavid

This could all be combined together: return (bool(matching_configs), repository, vcs_type, matching_configs)

daviddavid

F401 'reviewboard.reviews.signals.status_update_request_run' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.signals.status_update_request_run' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.signals.status_update_request_run' imported but unused

reviewbotreviewbot

These should use single quotes. Alternatively, Christian was suggesting we could do the import test once, up at the top ...

daviddavid

Given that we only need the should_build flag once now, let's remove it from the return tuple of this method ...

daviddavid

Can we clarify the method name here? Maybe test_manual_run_no_build_on_publish ?

daviddavid

F401 'importlib' imported but unused

reviewbotreviewbot

F401 'importlib' imported but unused

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

We still should have two blank lines here.

daviddavid

I believe there should be a period at the end of every comment.

jblazusijblazusi

I could be wrong about this when it is in regard to import statements, versus from ... import ... statements. ...

jblazusijblazusi

Please add a space between the = and \

daviddavid

Please put the """ on the next line here.

daviddavid

Unlike docstrings everywhere else, docstrings for test cases shouldn't end in a period (since the test runner will append an ...

daviddavid

Same here.

daviddavid

And here.

daviddavid

And here.

daviddavid
MarcusBoay
MarcusBoay
MarcusBoay
Review request changed

Change Summary:

Finalize this patch from [WIP] with unit tests

Summary:

-[WIP] Add manual trigger UI for CircleCI integration.
+Add manual trigger UI for CircleCI integration.

Description:

~  

[WIP] Add manual trigger UI for CircleCI integration.

  ~

Add manual trigger UI for CircleCI integration.

   
   

This change adds the ability for Integrations to run CircleCI manually.

    A checkbox is added to the CircleCI form that indicates if CircleCI should only
    run manually.

   
   

Based on work by David Trowbridge: /r/11130

  +
  +

This patch depends on /r/11190 and /r/11198

  + for the unit tests to pass.

Testing Done:

   

Manual testing by adding a private GitHub repository with CircleCI integration,

    checking and unchecking the "manually run" form checkbox to ensure that no
    automated runs and done when a review request is made, and clicking the "Run"
    button on the review request page.

   
~  

Unit tests are TBD.

  ~

Unit test have been written to verify that when the configuration is set,

  + the CircleCI request is not made.

Commits:

Summary
-
Add manual trigger UI for CircleCI integration.
+
Add manual trigger UI for CircleCI integration.
+
Add unit test for CircleCI manual run only.

Diff:

Revision 3 (+454 -104)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

MarcusBoay
MarcusBoay
MarcusBoay
MarcusBoay
MarcusBoay
MarcusBoay
MarcusBoay
MarcusBoay
MarcusBoay
david
  1. 
      
  2. rbintegrations/circleci/forms.py (Diff revision 9)
     
     

    You'll need to add a load method override to this form that conditionally disables the manual run field depending on the version of Review Board that it's running on. See the version in Review Bot for an example.

  3. rbintegrations/circleci/forms.py (Diff revision 9)
     
     

    Can we make the label "Run builds manually"?

  4. rbintegrations/circleci/integration.py (Diff revision 9)
     
     
     

    Why do we need to check if we should build again? It seems like if we shouldn't build the first time, we'd never even end up creating a status update which can be run/retried.

    1. You're right. This is an oversight on my part. I think I was going for a fail-safe in case things somehow slip through.

  5. rbintegrations/circleci/integration.py (Diff revision 9)
     
     
     

    Can we add a blank line between these two?

  6. rbintegrations/circleci/integration.py (Diff revision 9)
     
     

    This comment doesn't add anything useful (the code is pretty clear that it's initializing variables)

  7. rbintegrations/circleci/integration.py (Diff revision 9)
     
     

    Grammar here is a little off. It sounds like it should probably have a plural "requests": "Don't build any review requests that don't have matching configs"

  8. rbintegrations/circleci/integration.py (Diff revision 9)
     
     
     
     
     

    This could all be combined together:

    return (bool(matching_configs), repository, vcs_type,
            matching_configs)
    
  9. 
      
MarcusBoay
Review request changed

Change Summary:

Address feedback

Commits:

Summary
-
Add manual trigger UI for CircleCI integration.
-
Add unit test for CircleCI manual run only.
-
Refactor CircleCI integration code to reduce duplication.
-
Fix CircleCI diffset and add more tests.
+
Add manual trigger for CircleCI integration.

Diff:

Revision 10 (+648 -162)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

MarcusBoay
Review request changed

Change Summary:

Fix typo

Commits:

Summary
-
Add manual trigger for CircleCI integration.
+
Add manual trigger for CircleCI integration.

Diff:

Revision 11 (+648 -162)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

MarcusBoay
Review request changed

Change Summary:

Ignore unused import with inline comment

Commits:

Summary
-
Add manual trigger for CircleCI integration.
+
Add manual trigger for CircleCI integration.

Diff:

Revision 12 (+650 -162)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

MarcusBoay
david
  1. 
      
  2. rbintegrations/circleci/forms.py (Diff revision 13)
     
     
     

    These should use single quotes.

    Alternatively, Christian was suggesting we could do the import test once, up at the top of the file:

    try:
        from reviewboard.reviews.signals import status_update_request_run
    except ImportError:
        status_update_request_run = None
    

    and then here:

    if status_update_request_run is None:
        self.disabled_fields = ['run_manually']
        ...
    

    This has the advantage of only doing the import check once, though this form is very rarely accessed so it's probably not important either way. Your call if you want to change it.

  3. rbintegrations/circleci/integration.py (Diff revision 13)
     
     

    Given that we only need the should_build flag once now, let's remove it from the return tuple of this method and then where we use it, we can define it using bool(matching_configs).

    This is important here because of the _. Unfortunately for us, _ has two meanings: it's common to use it in python for unused return values, but we also often use it to name the ugettext import. This line could redefine the ugettext function as whatever the return value was (at least for the scope of this method). This is error-prone, so because we can avoid it entirely, we should.

    1. Interesting, thanks for informing me about the use of _.

  4. rbintegrations/circleci/tests.py (Diff revision 13)
     
     

    Can we clarify the method name here? Maybe test_manual_run_no_build_on_publish ?

  5. 
      
MarcusBoay
Review request changed

Change Summary:

Address David's comments

Commits:

Summary
-
Add manual trigger for CircleCI integration.
+
Add manual trigger for CircleCI integration.

Diff:

Revision 14 (+650 -164)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

MarcusBoay
Review request changed

Commits:

Summary
-
Add manual trigger for CircleCI integration.
+
Add manual trigger for CircleCI integration.

Diff:

Revision 15 (+650 -164)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

MarcusBoay
david
  1. Just a couple little style nits.

  2. rbintegrations/circleci/forms.py (Diff revision 16)
     
     
     

    We still should have two blank lines here.

  3. rbintegrations/circleci/integration.py (Diff revision 16)
     
     

    Please add a space between the = and \

  4. 
      
jblazusi
  1. Awesome Work!
    Unfortunately, I experienced a test failure for test #86 Testing circleCIIntegration build via a manual trigger.
    I am not sure why this is the case. The steps I took for this was:

    1) git checkout -b circleci from the master branch in rbintegrations repo.
    2) rbt patch -c 11208 successfully pulled downed the patch and committed the files.
    3) ./setup develop successfully ran
    4) rbtext test -m rbintegrations.circleci this is where I had a test fail, am I invoking this correctly?

    Otherwise, I just found some small styling comments, but I am not even completely sure whether they are correct.

    1. The test will be fixed once r/11190 is merged in together as it is dependent on it. Try:
      1) git checkout -b circleci from the master branch in rbintegration repository.
      3) rbt patch -c 11190
      2) rbt patch -c 11208
      4) ./setut develop
      5) rbext test -m rbintegrations.circleci

      Let me know if this works.

    2. I completely neglected the fact that your commit is based off of r/11190. After applying the patch, it worked perfectly. Thank you for the clarifcation. Everything looks good to me now.

  2. rbintegrations/circleci/forms.py (Diff revision 16)
     
     

    I believe there should be a period at the end of every comment.

  3. rbintegrations/circleci/integration.py (Diff revision 16)
     
     

    I could be wrong about this when it is in regard to import statements, versus from ... import ... statements. But I think that from datetime import datetime should come before import json to preserve alphabetical order. However, please correct me if I am wrong in this specific case.

    1. I grepped around all the repositories of reviewboard for from datetime and there are multiple instances where from datetime import datetime comes after import statements. For example, in djblets/djblets/cache/synchronizer.py.

  4. 
      
MarcusBoay
jblazusi
  1. Ship It!
  2. 
      
david
  1. 
      
  2. rbintegrations/circleci/tests.py (Diff revision 17)
     
     

    Please put the """ on the next line here.

  3. 
      
MarcusBoay
david
  1. One last nit.

  2. rbintegrations/circleci/tests.py (Diff revision 18)
     
     

    Unlike docstrings everywhere else, docstrings for test cases shouldn't end in a period (since the test runner will append an ellipsis when printing the test case name)

  3. rbintegrations/circleci/tests.py (Diff revision 18)
     
     

    Same here.

  4. rbintegrations/circleci/tests.py (Diff revision 18)
     
     

    And here.

  5. rbintegrations/circleci/tests.py (Diff revision 18)
     
     

    And here.

  6. 
      
MarcusBoay
david
  1. Ship It!
  2. 
      
MarcusBoay
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (69c5cd4)
Loading...