Add manual trigger run and retry for CircleCI integration.
Review Request #11208 — Created Oct. 1, 2020 and submitted
Information | |
---|---|
MarcusBoay | |
rbintegrations | |
master | |
Reviewers | |
rbintegrations | |
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 | |
---|---|
Description | From | Last Updated |
---|---|---|
W293 blank line contains whitespace |
![]() |
|
You'll need to add a load method override to this form that conditionally disables the manual run field depending on … |
|
|
Can we make the label "Run builds manually"? |
|
|
Why do we need to check if we should build again? It seems like if we shouldn't build the first … |
|
|
Can we add a blank line between these two? |
|
|
This comment doesn't add anything useful (the code is pretty clear that it's initializing variables) |
|
|
Grammar here is a little off. It sounds like it should probably have a plural "requests": "Don't build any review … |
|
|
This could all be combined together: return (bool(matching_configs), repository, vcs_type, matching_configs) |
|
|
F401 'reviewboard.reviews.signals.status_update_request_run' imported but unused |
![]() |
|
F401 'reviewboard.reviews.signals.status_update_request_run' imported but unused |
![]() |
|
F401 'reviewboard.reviews.signals.status_update_request_run' imported but unused |
![]() |
|
These should use single quotes. Alternatively, Christian was suggesting we could do the import test once, up at the top … |
|
|
Given that we only need the should_build flag once now, let's remove it from the return tuple of this method … |
|
|
Can we clarify the method name here? Maybe test_manual_run_no_build_on_publish ? |
|
|
F401 'importlib' imported but unused |
![]() |
|
F401 'importlib' imported but unused |
![]() |
|
E501 line too long (80 > 79 characters) |
![]() |
|
We still should have two blank lines here. |
|
|
I believe there should be a period at the end of every comment. |
![]() |
|
I could be wrong about this when it is in regard to import statements, versus from ... import ... statements. … |
![]() |
|
Please add a space between the = and \ |
|
|
Please put the """ on the next line here. |
|
|
Unlike docstrings everywhere else, docstrings for test cases shouldn't end in a period (since the test runner will append an … |
|
|
Same here. |
|
|
And here. |
|
|
And here. |
|
Description: |
|
---|
Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+378 -102) |
Checks run (2 succeeded)
Change Summary:
Finalize this patch from [WIP] with unit tests
Summary: |
|
|||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||
Commits: |
|
|||||||||||||||||||||||||||||||||
Diff: |
Revision 3 (+454 -104) |
Checks run (1 failed, 1 succeeded)
flake8
-
rbintegrations/circleci/tests.py (Diff revision 3) Show all issues
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+454 -104) |
Checks run (2 succeeded)
Description: |
|
---|
Change Summary:
Update testing description with RB3 and RB4
Description: |
|
||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Change Summary:
Refactor code to reduce code duplication
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+528 -210) |
Checks run (2 succeeded)
Change Summary:
Add missing documentation
Commits: |
|
||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+536 -212) |
Checks run (2 succeeded)
Change Summary:
Fix types in docstrings
Commits: |
|
||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+538 -214) |
Checks run (2 succeeded)
Change Summary:
Fix diffset for manual run and add test for manual trigger.
Testing Done: |
|
|||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
|||||||||||||||||||||||||||||||||
Diff: |
Revision 8 (+690 -224) |
Checks run (2 succeeded)
Change Summary:
Minor documentation fixup
Commits: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+692 -226) |
Checks run (2 succeeded)
Change Summary:
Attached demo video in Description
Description: |
|
---|
-
-
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. -
-
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.
-
-
rbintegrations/circleci/integration.py (Diff revision 9) This comment doesn't add anything useful (the code is pretty clear that it's initializing variables)
-
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"
-
rbintegrations/circleci/integration.py (Diff revision 9) This could all be combined together:
return (bool(matching_configs), repository, vcs_type, matching_configs)
Change Summary:
Address feedback
Commits: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+648 -162) |
Checks run (1 failed, 1 succeeded)
flake8
-
rbintegrations/circleci/forms.py (Diff revision 10) F401 'reviewboard.reviews.signals.status_update_request_run' imported but unused
Change Summary:
Fix typo
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+648 -162) |
Checks run (1 failed, 1 succeeded)
flake8
-
rbintegrations/circleci/forms.py (Diff revision 11) F401 'reviewboard.reviews.signals.status_update_request_run' imported but unused
Change Summary:
Ignore unused import with inline comment
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 12 (+650 -162) |
Checks run (1 failed, 1 succeeded)
flake8
-
rbintegrations/circleci/forms.py (Diff revision 12) F401 'reviewboard.reviews.signals.status_update_request_run' imported but unused
Change Summary:
Fix typo in can_retry extra_data
Summary: |
|
|||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||
Commits: |
|
|||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 13 (+654 -162) |
Checks run (2 succeeded)
-
-
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.
-
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 usingbool(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. -
rbintegrations/circleci/tests.py (Diff revision 13) Can we clarify the method name here? Maybe
test_manual_run_no_build_on_publish
?
Change Summary:
Address David's comments
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 14 (+650 -164) |
Checks run (1 failed, 1 succeeded)
flake8
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 15 (+650 -164) |
Checks run (1 failed, 1 succeeded)
flake8
Change Summary:
Address ReviewBot issues
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 16 (+648 -164) |
Checks run (2 succeeded)
-
Just a couple little style nits.
-
-

-
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.
-
rbintegrations/circleci/forms.py (Diff revision 16) I believe there should be a period at the end of every comment.
-
rbintegrations/circleci/integration.py (Diff revision 16) I could be wrong about this when it is in regard to
import
statements, versusfrom ... import ...
statements. But I think thatfrom datetime import datetime
should come beforeimport json
to preserve alphabetical order. However, please correct me if I am wrong in this specific case.
Change Summary:
Address feedback
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 17 (+648 -162) |
Checks run (2 succeeded)
Change Summary:
Adjust docstrings
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 18 (+656 -166) |
Checks run (2 succeeded)
-
One last nit.
-
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)
-
-
-
Change Summary:
Removed period at the end of docstrings for tests
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 19 (+654 -164) |