Review Request #8951 — Created May 19, 2017 and submitted
This change adds a new integration which allows people to build prospective
changes via CircleCI. This requires the (beta) CircleCI 2.0.This is similar in many ways to the Travis CI integration, but is somewhat
different in its configuration. The major difference is that while Travis
allows us to override the config within our API request, Circle does not. This
means that users will have to add Review Board support within their
.circleci/config.ymlfile.When a config matches, this integration will trigger a build of the
"reviewboard" job (as defined in the user's config) and pass along various bits
of data in the build parameters.
Posted a variety of changes which should either fail or pass the build, and
saw the results reported correctly.
| Description | From | Last Updated |
|---|---|---|
|
E251 unexpected spaces around keyword / parameter equals |
 reviewbot
|
|
|
E251 unexpected spaces around keyword / parameter equals |
reviewbot
|
|
|
E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
E999 SyntaxError: invalid syntax |
reviewbot
|
|
|
F841 local variable 'config' is assigned to but never used |
reviewbot
|
|
|
reviewboard.reviews.models.review_request.ReviewRequest. It won't link otherwise. You can break the line at the . now though. |
 chipx86
|
|
|
Does CircleCI support GitHub Enterprise? |
chipx86
|
|
|
Why not put this in the argument list? |
chipx86
|
|
|
Since the user could in theory have multiple tokens, we should probably handle that case here so things don't break. |
chipx86
|
|
|
urlquote_plus will fail if circle_api_token is None (missing in config). |
chipx86
|
|
|
Do we need to send this? Seems the Local Site + display ID would be enough. I'd rather not expose … |
chipx86
|
|
|
How does this user work when using Local Sites? |
chipx86
|
|
|
There's potential for a race condition here, where in-between the previous lookup and now another thread created this user. We … |
chipx86
|
|
|
Blank line before the comment. |
chipx86
|
|
|
We need unit tests handling Local Site scenarios. |
chipx86
|
|
|
*args |
chipx86
|
|
|
**kwargs |
chipx86
|
|
|
Let's dump the payload so we have something to go off of for debugging. |
chipx86
|
|
|
As above, it would be nice to use the display ID + Local Site name, instead of passing around the … |
chipx86
|
|
|
Display ID + Local Site name |
chipx86
|
|
|
Missing period. |
chipx86
|
|
|
We're still exposing the review request ID. We should be purely using the display ID + local site, and never … |
chipx86
|
|
|
Why _response? Not _Response? |
chipx86
|
|
|
Having all the content on one line will help tie things together in the logs, since otherwise entries can become … |
chipx86
|
| rbintegrations/circleci/integration.py (Diff revision 1) |
|---|
| rbintegrations/circleci/integration.py (Diff revision 1) |
|---|
E251 unexpected spaces around keyword / parameter equals
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+447 -41) |
Add tests for basic behavior. This requires fewer tests than Travis right now because the config on the RB side is quite a bit simpler.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 3 (+596 -41) |
| rbintegrations/circleci/tests.py (Diff revision 3) |
|---|
F841 local variable 'config' is assigned to but never used
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+596 -41) |
| rbintegrations/circleci/integration.py (Diff revision 4) |
|---|
reviewboard.reviews.models.review_request.ReviewRequest. It won't link otherwise. You can break the line at the.now though.
| rbintegrations/circleci/integration.py (Diff revision 4) |
|---|
Since the user could in theory have multiple tokens, we should probably handle that case here so things don't break.
| rbintegrations/circleci/integration.py (Diff revision 4) |
|---|
urlquote_pluswill fail ifcircle_api_tokenis None (missing inconfig).
| rbintegrations/circleci/integration.py (Diff revision 4) |
|---|
Do we need to send this? Seems the Local Site + display ID would be enough. I'd rather not expose this if we can avoid it.
| rbintegrations/circleci/integration.py (Diff revision 4) |
|---|
How does this user work when using Local Sites?
| rbintegrations/circleci/integration.py (Diff revision 4) |
|---|
There's potential for a race condition here, where in-between the previous lookup and now another thread created this user. We should have a
try/exceptaround this and return the existing entry if we get anIntegrityError.
| rbintegrations/circleci/tests.py (Diff revision 4) |
|---|
We need unit tests handling Local Site scenarios.
| rbintegrations/circleci/views.py (Diff revision 4) |
|---|
Let's dump the payload so we have something to go off of for debugging.
| rbintegrations/circleci/views.py (Diff revision 4) |
|---|
As above, it would be nice to use the display ID + Local Site name, instead of passing around the database ID.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+666 -41) |
| rbintegrations/circleci/integration.py (Diff revision 5) |
|---|
We're still exposing the review request ID. We should be purely using the display ID + local site, and never expose the review request ID itself.
| rbintegrations/circleci/views.py (Diff revision 5) |
|---|
Having all the content on one line will help tie things together in the logs, since otherwise entries can become interspersed when aggregated.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+666 -41) |
