Summary

Add integration for CircleCI.

Review Request #8951 — Created May 19, 2017 and submitted July 14, 2017, 4:22 p.m.

Information

Owner

david

Repository

rbintegrations

Branch

master

Bugs

Depends On

Commit

92bed80...

Reviewers

Groups

rbintegrations

People

Description

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.yml file.

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.

Testing Done

Posted a variety of changes which should either fail or pass the build, and
saw the results reported correctly.

Issues

Description	From	Last Updated
E251 unexpected spaces around keyword / parameter equals	reviewbot	May 19, 2017, 2:11 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	May 19, 2017, 2:11 p.m.
E501 line too long (82 > 79 characters)	reviewbot	May 19, 2017, 2:11 p.m.
E999 SyntaxError: invalid syntax	reviewbot	May 19, 2017, 2:11 p.m.
F841 local variable 'config' is assigned to but never used	reviewbot	May 22, 2017, 4:26 p.m.
reviewboard.reviews.models.review_request.ReviewRequest. It won't link otherwise. You can break the line at the . now though.	chipx86	June 15, 2017, 11:51 a.m.
Does CircleCI support GitHub Enterprise?	chipx86	June 15, 2017, 11:52 a.m.
Why not put this in the argument list?	chipx86	June 15, 2017, 11:53 a.m.
Since the user could in theory have multiple tokens, we should probably handle that case here so things don't break.	chipx86	June 15, 2017, 11:55 a.m.
urlquote_plus will fail if circle_api_token is None (missing in config).	chipx86	June 15, 2017, 11:55 a.m.
Do we need to send this? Seems the Local Site + display ID would be enough. I'd rather not expose …	chipx86	June 28, 2017, 1:26 p.m.
How does this user work when using Local Sites?	chipx86	June 28, 2017, 1:54 p.m.
There's potential for a race condition here, where in-between the previous lookup and now another thread created this user. We …	chipx86	June 15, 2017, 11:57 a.m.
Blank line before the comment.	chipx86	June 15, 2017, 11:57 a.m.
We need unit tests handling Local Site scenarios.	chipx86	June 28, 2017, 1:54 p.m.
*args	chipx86	June 15, 2017, 12:36 p.m.
**kwargs	chipx86	June 15, 2017, 12:36 p.m.
Let's dump the payload so we have something to go off of for debugging.	chipx86	June 15, 2017, 12:36 p.m.
As above, it would be nice to use the display ID + Local Site name, instead of passing around the …	chipx86	June 28, 2017, 1:26 p.m.
Display ID + Local Site name	chipx86	June 28, 2017, 1:26 p.m.
Missing period.	chipx86	July 10, 2017, 10:50 a.m.
We're still exposing the review request ID. We should be purely using the display ID + local site, and never …	chipx86	July 10, 2017, 10:51 a.m.
Why _response? Not _Response?	chipx86	July 10, 2017, 10:50 a.m.
Having all the content on one line will help tie things together in the logs, since otherwise entries can become …	chipx86	July 10, 2017, 10:51 a.m.

flake8 failed.

JSHint passed.

flake8

rbintegrations/circleci/integration.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
rbintegrations/circleci/integration.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
rbintegrations/circleci/integration.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E501 line too long (82 > 79 characters)
```
rbintegrations/circleci/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E999 SyntaxError: invalid syntax
```

Commit:

309cf6424e1a766f1d9d81a64215aa26729b121d

169f7dff8dbcd9645e183533b3ae006cbff98c41

Diff:

Revision 2 (+447 -41)

Show changes

	rbintegrations/extension.py
	rbintegrations/circleci/__init__.py
	rbintegrations/circleci/forms.py
	rbintegrations/circleci/integration.py
	rbintegrations/circleci/urls.py
	rbintegrations/circleci/views.py
	rbintegrations/static/images/circleci/icon.png
	rbintegrations/static/images/circleci/icon@2x.png
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

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:

		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.yml` file.

		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.
-
-		Documentation and unit tests will be forthcoming in a future change.

Commit:

169f7dff8dbcd9645e183533b3ae006cbff98c41

0436b0ed49484262b222af443b8d935447a50e8a

Diff:

Revision 3 (+596 -41)

Show changes

	rbintegrations/extension.py
	rbintegrations/circleci/__init__.py
	rbintegrations/circleci/forms.py
	rbintegrations/circleci/integration.py
	rbintegrations/circleci/tests.py
	rbintegrations/circleci/urls.py
	rbintegrations/circleci/views.py
	rbintegrations/static/images/circleci/icon.png
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbintegrations/circleci/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F841 local variable 'config' is assigned to but never used
```

Commit:

0436b0ed49484262b222af443b8d935447a50e8a

ba0dbcb867a15501384e391a60b26e30f70410ed

Diff:

Revision 4 (+596 -41)

Show changes

	rbintegrations/extension.py
	rbintegrations/circleci/__init__.py
	rbintegrations/circleci/forms.py
	rbintegrations/circleci/integration.py
	rbintegrations/circleci/tests.py
	rbintegrations/circleci/urls.py
	rbintegrations/circleci/views.py
	rbintegrations/static/images/circleci/icon.png
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

The issue has been resolved. Show all issues

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)
The issue has been dropped. Show all issues
```
Does CircleCI support GitHub Enterprise?
```
1. david June 28, 2017, 1:58 p.m.
  Not that I can tell.
rbintegrations/circleci/integration.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Why not put this in the argument list?
```

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

The issue has been resolved. Show all issues

Since the user could in theory have multiple tokens, we should probably handle that case here so things don't break.

david June 28, 2017, 1:58 p.m.

This user is programmatically created by the extension, but I guess I can bullet-proof it a bit.

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

The issue has been dropped. Show all issues

urlquote_plus will fail if circle_api_token is None (missing in config).

david June 28, 2017, 1:58 p.m.

The API token isn't an optional field when saving the config.

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

The issue has been resolved. Show all issues

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)
The issue has been resolved. Show all issues
```
How does this user work when using Local Sites?
```

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

The issue has been resolved. Show all issues

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/except around this and return the existing entry if we get an IntegrityError.

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

The issue has been dropped. Show all issues

Blank line before the comment.

david June 28, 2017, 1:58 p.m.

Ehhhh. This is a TODO comment interjected between two lines that would otherwise be adjacent. Once this comment goes away there won't be a blank line there.

rbintegrations/circleci/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
We need unit tests handling Local Site scenarios.
```
rbintegrations/circleci/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
*args
```
rbintegrations/circleci/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
**kwargs
```
rbintegrations/circleci/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Let's dump the payload so we have something to go off of for debugging.
```

rbintegrations/circleci/views.py (Diff revision 4)

The issue has been resolved. Show all issues

As above, it would be nice to use the display ID + Local Site name, instead of passing around the database ID.

rbintegrations/circleci/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Display ID + Local Site name
```

Commit:

ba0dbcb867a15501384e391a60b26e30f70410ed

dfc94e5f7092211345a3326fbdbe638f7474a63c

Diff:

Revision 5 (+666 -41)

Show changes

	rbintegrations/extension.py
	rbintegrations/circleci/__init__.py
	rbintegrations/circleci/forms.py
	rbintegrations/circleci/integration.py
	rbintegrations/circleci/tests.py
	rbintegrations/circleci/urls.py
	rbintegrations/circleci/views.py
	rbintegrations/static/images/circleci/icon.png
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbintegrations/circleci/forms.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Missing period.
```

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

The issue has been resolved. Show all issues

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.

david July 10, 2017, 10:51 a.m.
```
Just forgot to delete this line.
```

rbintegrations/circleci/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Why _response? Not _Response?
```

rbintegrations/circleci/views.py (Diff revision 5)

The issue has been resolved. Show all issues

Having all the content on one line will help tie things together in the logs, since otherwise entries can become interspersed when aggregated.

Commit:

dfc94e5f7092211345a3326fbdbe638f7474a63c

92bed8091386e0d95bdfc9a74d7fd21fba67d095

Diff:

Revision 6 (+666 -41)

Show changes

	rbintegrations/extension.py
	rbintegrations/circleci/__init__.py
	rbintegrations/circleci/forms.py
	rbintegrations/circleci/integration.py
	rbintegrations/circleci/tests.py
	rbintegrations/circleci/urls.py
	rbintegrations/circleci/views.py
	rbintegrations/static/images/circleci/icon.png
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
This LGTM!
```

Status:: Completed
Change Summary:: Pushed to master (25d4cb8)