Summary

Add an integration for Travis CI.

Review Request #8921 — Created May 5, 2017 and submitted July 14, 2017, 4:21 p.m.

Information

Owner

david

Repository

rbintegrations

Branch

master

Bugs

Depends On

Commit

c032765...

Reviewers

Groups

rbintegrations

People

Description

This change adds a new integration which allows people to build prospective
changes via Travis CI.

The Travis CI API allows triggering a build in a way that overrides the config
from the repository's .travis.yml file. We use this to send a new
configuration which knows how to apply the patch and report the results back to
Review Board.

The configuration for the integration allows users to set the conditions
normally, with the exception that the choices for the "Repository" condition
are limited to only show GitHub repositories (since Travis CI doesn't work with
anything else). It also requires selecting which Travis endpoint to use (either
travis-ci.org for open-source projects, travis-ci.com for private projects, or
an enterprise server with a custom domain), along with providing an API token.

Currently, builds show up under the main "Branches" section of the repository
in the Travis CI user interface. Because this can pollute things (for example,
showing that the master branch is broken because a build of a review request
failed), for now I'm recommending that people create a fake review-requests
branch on their GitHub remote and then set that branch name in the integration
config. This isn't perfect, but it at least keeps review requests builds
separate. I'm starting to talk to the Travis CI folks to see if we can get some
more control through the API, perhaps even letting us list these builds in the
"Pull Requests" section instead of the "Branches" section.

The modifications made to the user-entered configuration involve changing the
script section to insert a few commands to check out the correct parent
revision and apply the patch, and modifications to the notifications section
to disable e-mail notification and set up a webhook to receive results. For
example, if the build config was set to the following:

language: python
python:
    - 2.7
env:
    - DJANGO_VERSION=1.6.11
    - DJANGO_VERSION=1.8.2
install:
    - python setup.py develop
    - pip install -r dev-requirements.txt
script:
    - python ./tests/runtests.py

Then the resulting configuration that would be sent to Travis would look
something like this:

language: python
python:
    - 2.7
env:
    global:
        - REVIEWBOARD_STATUS_UPDATE_ID=71
        - REVIEWBOARD_TRAVIS_INTEGRATION_CONFIG_ID=4
    matrix:
        - DJANGO_VERSION=1.6.11
        - DJANGO_VERSION=1.8.2
install:
    - python setup.py develop
    - pip install -r dev-requirements.txt
merge_mode: replace
notifications:
    email: false
    webhooks:
        on_start: always
        urls:
            - http://reviewboard.example.com/rbintegrations/travis-ci/webhook/
script:
    - git fetch --unshallow origin
    - git checkout 5ec2912cd1cad96a3a20ea5857e9ed2d1de0d465
    - echo <big base64 blob> | base64 --decode | patch -p1
    - python ./tests/runtests.py

Testing Done

Integration configuration:

Tested that only GitHub repositories showed up under the "Repository"
condition.
Tested interaction where the server name field was hidden by default and only
shown when the "Travis CI" field was set to "Enterprise (custom domain)".
Verified that a correct error message was shown if the API token was
incorrect.
Verified that an incorrect configuration would show the proper errors as
reported by the Travis CI lint API.

Usage:

Posted a variety of changes which should either fail or pass the build, and
saw the results reported correctly.
Verified that webhook signature validation was correct, passing in regular
use and failing if I made any changes to the payload contents.

Files

Issues

Description	From	Last Updated
Small typo in the example generated configuration. The comma shouldn't be there after the DJANGO_VERSION=1.6.11.	chipx86	June 28, 2017, 9:59 a.m.
This help text should probs be aligned with the description above.	brennie	July 14, 2017, 3:17 p.m.
Alignment here is off.	brennie	July 14, 2017, 3:17 p.m.
We definitely should have a warning that they should omit secret environment variables, because people will be able to print …	brennie	July 14, 2017, 3:22 p.m.
Col: 15 'selectedEndpoint' is defined but never used.	reviewbot	May 5, 2017, 2:09 p.m.
'django.utils.six.moves.urllib.error.URLError' imported but unused	reviewbot	May 5, 2017, 2:09 p.m.
local variable 'url' is assigned to but never used	reviewbot	May 5, 2017, 2:09 p.m.
'django.core.exceptions.ValidationError' imported but unused	reviewbot	May 5, 2017, 2:10 p.m.
local variable 'section_name' is assigned to but never used	reviewbot	May 5, 2017, 2:10 p.m.
'json' imported but unused	reviewbot	May 5, 2017, 2:10 p.m.
local variable 'review_request_id' is assigned to but never used	reviewbot	May 5, 2017, 2:10 p.m.
local variable 'data' is assigned to but never used	reviewbot	May 5, 2017, 2:10 p.m.
Any reason you don't use an arrow func here?	brennie	May 6, 2017, 8:29 p.m.
Module docstring	brennie	May 8, 2017, 10:01 a.m.
Can we not just use the one from hostingsvcs to avoid duplicate code?	brennie	May 7, 2017, 12:16 a.m.
"HTTP GET"	brennie	May 8, 2017, 10:01 a.m.
Should be indented.	brennie	May 8, 2017, 10:01 a.m.
Needs to be qualified.	brennie	May 8, 2017, 10:01 a.m.
Should be indented.	brennie	May 8, 2017, 10:02 a.m.
Needs to be qualified.	brennie	May 8, 2017, 10:02 a.m.
Needs to be qualified.	brennie	May 8, 2017, 10:03 a.m.
Needs to be qualified.	brennie	May 8, 2017, 10:03 a.m.
Should be indented.	brennie	May 8, 2017, 10:03 a.m.
Needs to be qualified.	brennie	May 8, 2017, 10:03 a.m.
Should be indented.	brennie	May 8, 2017, 10:03 a.m.
Module level docstring.	brennie	May 8, 2017, 10:03 a.m.
Don't we usually have these at top of funcs?	brennie	May 8, 2017, 10:04 a.m.
Django behaviour is to remove error'ed fields from cleaned_data. Shouldn't this be a ValidationError(_(...)) since self.error_class is ErrorList ? Same …	brennie	May 8, 2017, 11:30 a.m.
Module level docstring.	brennie	May 8, 2017, 10:04 a.m.
Can we distribute these?	brennie	May 6, 2017, 8:30 p.m.
Rather, review requests that are attachment-only.	brennie	May 9, 2017, 3:38 p.m.
This isn't Review Bot.	brennie	May 8, 2017, 10:05 a.m.
Module level docstring.	brennie	May 8, 2017, 10:05 a.m.
No blank line here.	brennie	May 8, 2017, 10:05 a.m.
Blank line between these.	brennie	May 8, 2017, 10:05 a.m.
W503 line break before binary operator	reviewbot	May 8, 2017, 11:41 a.m.
You still need to pop travis_custom_endpoint out of self._errors. Same below.	brennie	May 10, 2017, 2:08 p.m.
E127 continuation line over-indented for visual indent	reviewbot	May 8, 2017, 11:41 a.m.
E501 line too long (83 > 79 characters)	reviewbot	May 8, 2017, 11:41 a.m.
W503 line break before binary operator	reviewbot	May 10, 2017, 2:07 p.m.
E127 continuation line over-indented for visual indent	reviewbot	May 10, 2017, 2:08 p.m.
E501 line too long (83 > 79 characters)	reviewbot	May 10, 2017, 2:08 p.m.
Can we use travis-ci instead of travisci?	chipx86	June 28, 2017, 10:50 a.m.
You can do: $server.setVisible($endpoint.val() === 'E');	chipx86	June 28, 2017, 10 a.m.
Maybe condense these to be two lines? u = urlopen(URLRequest(...)) return json.loads(u.read())	chipx86	June 28, 2017, 10 a.m.
dict?	chipx86	June 28, 2017, 10 a.m.
reviewboard.reviews.models.review_request.ReviewRequest	chipx86	June 28, 2017, 10:01 a.m.
Maybe we can use the existing list and perform a - [ReviewRequestRepositoryTypeChoice] (or whatever)?	chipx86	June 28, 2017, 11 a.m.
and on the previous line.	chipx86	July 8, 2017, 11:59 a.m.
W503 line break before binary operator	reviewbot	May 19, 2017, 2:12 p.m.
We should probably use six.text_type(e) instead of e directly. Same below.	chipx86	June 28, 2017, 10:02 a.m.
Can simplify this a bit: if warning['key']: ... message = ... else: message = warning['message'] self._errors['travis_yml'] = self.error_class([message])	chipx86	June 28, 2017, 10:03 a.m.
Can we log with request=self.request on all these logging statements, so we'll have user/Local Site context?	chipx86	June 28, 2017, 10:04 a.m.
repository's Let's also just use double quotes for this string instead of escaping the apostrophes.	chipx86	June 28, 2017, 10:05 a.m.
Let's pull out the repository into a variable. It'll prevent the Review Bot complaint, since it should be able to …	chipx86	June 28, 2017, 10:06 a.m.
E127 continuation line over-indented for visual indent	reviewbot	May 19, 2017, 2:12 p.m.
We can include this in the function.	chipx86	June 28, 2017, 10:07 a.m.
Can these be: notifications = travis_config.get('notifications') or {} ?	chipx86	June 28, 2017, 10:07 a.m.
Can use += [ ... ]	chipx86	June 28, 2017, 10:07 a.m.
We're going to need to check for IntegrityErrors, in case another process beat this one to the punch on creating …	chipx86	June 28, 2017, 10:15 a.m.
*args	chipx86	June 28, 2017, 10:16 a.m.
**kwargs	chipx86	June 28, 2017, 10:16 a.m.
E501 line too long (83 > 79 characters)	reviewbot	May 19, 2017, 2:12 p.m.
You can wrap after the . anywhere now.	chipx86	June 28, 2017, 10:16 a.m.
This will crash if HTTP_SIGNATURE isn't present.	chipx86	June 28, 2017, 10:17 a.m.
Should be bullet-proof this? Log an error if we get a KeyError?	chipx86	June 28, 2017, 10:17 a.m.
Does this raise an exception if any part of this fails?	chipx86	June 28, 2017, 10:18 a.m.
We should bullet-proof this as well.	chipx86	June 28, 2017, 10:18 a.m.
W503 line break before binary operator	reviewbot	June 28, 2017, 12:27 p.m.
E501 line too long (81 > 79 characters)	reviewbot	June 28, 2017, 12:27 p.m.
E501 line too long (83 > 79 characters)	reviewbot	June 28, 2017, 12:27 p.m.
E122 continuation line missing indentation or outdented	reviewbot	June 28, 2017, 12:27 p.m.
You can pass the method in directory. There's no this being used.	chipx86	July 8, 2017, noon
We should guard against the request failing or the data not being parsable. (Proxy servers may get in the way …	chipx86	July 8, 2017, 12:23 p.m.
"GitHub"	chipx86	July 8, 2017, noon
GitHub	brennie	July 8, 2017, 12:18 p.m.
Can you wrap these in parens so it's a little more visually clear what's happening? Since this is Python 2.7, …	chipx86	July 8, 2017, 12:18 p.m.
W503 line break before binary operator	reviewbot	June 28, 2017, 12:36 p.m.
Should this be marked for translation?	brennie	July 8, 2017, 12:21 p.m.
Do we want to limit the scope of this token?	brennie	July 8, 2017, 1:27 p.m.
E501 line too long (83 > 79 characters)	reviewbot	June 28, 2017, 12:36 p.m.
Sine this line's too long, and we dig into cryptography.hazmat.primitives a bunch, let's just pull primitives out.	chipx86	July 8, 2017, 12:22 p.m.
W503 line break before binary operator	reviewbot	July 8, 2017, 1:54 p.m.

JSHint failed.

PEP8 Style Checker internal error.

Pyflakes failed.

JSHint

rbintegrations/static/js/travisci/integrationConfig.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 15
 'selectedEndpoint' is defined but never used.
```

Pyflakes

rbintegrations/travisci/api.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'django.utils.six.moves.urllib.error.URLError' imported but unused
```
rbintegrations/travisci/api.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 local variable 'url' is assigned to but never used
```
rbintegrations/travisci/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'django.core.exceptions.ValidationError' imported but unused
```
rbintegrations/travisci/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 local variable 'section_name' is assigned to but never used
```
rbintegrations/travisci/integration.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'json' imported but unused
```
rbintegrations/travisci/integration.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 local variable 'review_request_id' is assigned to but never used
```
rbintegrations/travisci/integration.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 local variable 'data' is assigned to but never used
```

Commit:

-	c98d7f3ee064cc1ddf4fc489854f401f2d482289
+	c0112d02f0b7c92c854ffc9a1076ba2d0b916bc5

Diff:

Revision 2 (+949 -1)

Show changes

	setup.py
	rbintegrations/extension.py
	rbintegrations/static/css/travisci/integration-config.less
	rbintegrations/static/images/travisci/icon.png
	rbintegrations/static/images/travisci/icon@2x.png
	rbintegrations/static/js/travisci/integrationConfig.es6.js
	rbintegrations/travisci/__init__.py
	rbintegrations/travisci/api.py
	4 more

Checks run (2 succeeded, 1 failed with error)

JSHint passed.

PEP8 Style Checker internal error.

Pyflakes passed.

rbintegrations/static/js/travisci/integrationConfig.es6.js (Diff revision 2)

The issue has been dropped. Show all issues

Any reason you don't use an arrow func here?

david May 6, 2017, 8:32 p.m.

There's no real reason to use it (we don't care about lexical this and it doesn't need to be that compact), so we might as well avoid the extra overhead added by the transpilation.

rbintegrations/travisci/api.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Module docstring
```

rbintegrations/travisci/api.py (Diff revision 2)

The issue has been dropped. Show all issues

Can we not just use the one from hostingsvcs to avoid duplicate code?

david May 6, 2017, 8:32 p.m.

It seemed super weird to be to be using something which is specific to hosting services inside this. If you moved it somewhere more common (djblets?) I'd be happy to use it.

brennie May 8, 2017, 12:19 p.m.

Yeah, at this point it should probably live there (and mayb be renamed to just Request instead of URLRequest)

rbintegrations/travisci/api.py (Diff revision 2)
The issue has been resolved. Show all issues
```
"HTTP GET"
```
rbintegrations/travisci/api.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should be indented.
```
rbintegrations/travisci/api.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Needs to be qualified.
```
rbintegrations/travisci/api.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should be indented.
```
rbintegrations/travisci/api.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Needs to be qualified.
```
rbintegrations/travisci/api.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Needs to be qualified.
```
rbintegrations/travisci/api.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Needs to be qualified.
```
rbintegrations/travisci/api.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should be indented.
```
rbintegrations/travisci/api.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Needs to be qualified.
```
rbintegrations/travisci/api.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should be indented.
```
rbintegrations/travisci/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Module level docstring.
```
rbintegrations/travisci/forms.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Don't we usually have these at top of funcs?
```
1. david May 8, 2017, 11:30 a.m.
  No? I prefer to import things adjacent to where they're used.

rbintegrations/travisci/forms.py (Diff revision 2)

The issue has been dropped. Show all issues

Django behaviour is to remove error'ed fields from cleaned_data.
Shouldn't this be a ValidationError(_(...)) since self.error_class is ErrorList ?
Same below.

david May 8, 2017, 11:30 a.m.

Based on the docs and functionality, this is correct. The normal form class catches ValidationError and extracts the message to put it into the ErrorList.

rbintegrations/travisci/integration.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Module level docstring.
```
rbintegrations/travisci/integration.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Can we distribute these?
```
1. david May 6, 2017, 8:32 p.m.
  Yep, so long as we follow their normal branding guidelines.

rbintegrations/travisci/integration.py (Diff revision 2)

The issue has been dropped. Show all issues

Rather, review requests that are attachment-only.

david May 6, 2017, 8:32 p.m.

That's not the case. We already checked that the review request has a repository. In this case we're checking that if it's an update, that the update added a new diff.

brennie May 8, 2017, 12:19 p.m.

Yes but wouldn't get_latest_diffset() return the latest diffset, not the diffset from the latest change? ie, get_latest_diffset() is None should be mutually exclusive with review_requet.repository being not None.
The change for the latest change having a diff comes after when you test for 'diff' in fields_changed.

david May 9, 2017, 3:38 p.m.

I made this comment better in the latest diff.

rbintegrations/travisci/integration.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This isn't Review Bot.
```
rbintegrations/travisci/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Module level docstring.
```
rbintegrations/travisci/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No blank line here.
```
rbintegrations/travisci/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between these.
```

Commit:

-	c0112d02f0b7c92c854ffc9a1076ba2d0b916bc5
+	8fd69d70f07b57c21ad8733c1c04ae604d21493f

Diff:

Revision 3 (+961 -1)

Show changes

	setup.py
	rbintegrations/extension.py
	rbintegrations/static/css/travisci/integration-config.less
	rbintegrations/static/images/travisci/icon.png
	rbintegrations/static/images/travisci/icon@2x.png
	rbintegrations/static/js/travisci/integrationConfig.es6.js
	rbintegrations/travisci/__init__.py
	rbintegrations/travisci/api.py
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbintegrations/travisci/forms.py (Diff revision 3)
The issue has been dropped. Show all issues
```
W503 line break before binary operator
```
rbintegrations/travisci/integration.py (Diff revision 3)
The issue has been dropped. Show all issues
```
E127 continuation line over-indented for visual indent
```
rbintegrations/travisci/views.py (Diff revision 3)
The issue has been dropped. Show all issues
```
E501 line too long (83 > 79 characters)
```

rbintegrations/travisci/forms.py (Diff revision 3)

The issue has been dropped. Show all issues

You still need to pop travis_custom_endpoint out of self._errors. Same below.

brennie May 8, 2017, 12:19 p.m.

Rather, out of self.cleaned_data. Haven't had my coffee yet.

david May 9, 2017, 4:20 p.m.

Why is that? This doesn't do any additional processing on cleaned_data once validation fails.

Change Summary:

Some random fixes and improvements that came up as I was writing tests.

Commit:

-	8fd69d70f07b57c21ad8733c1c04ae604d21493f
+	07a5b58bfa7efe5c5880dec5995ac12f557383bc

Diff:

Revision 4 (+962 -1)

Show changes

	setup.py
	rbintegrations/extension.py
	rbintegrations/static/css/travisci/integration-config.less
	rbintegrations/static/images/travisci/icon.png
	rbintegrations/static/images/travisci/icon@2x.png
	rbintegrations/static/js/travisci/integrationConfig.es6.js
	rbintegrations/travisci/__init__.py
	rbintegrations/travisci/api.py
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbintegrations/travisci/forms.py (Diff revision 4)
The issue has been dropped. Show all issues
```
W503 line break before binary operator
```
rbintegrations/travisci/integration.py (Diff revision 4)
The issue has been dropped. Show all issues
```
E127 continuation line over-indented for visual indent
```
rbintegrations/travisci/views.py (Diff revision 4)
The issue has been dropped. Show all issues
```
E501 line too long (83 > 79 characters)
```

Commit:

-	07a5b58bfa7efe5c5880dec5995ac12f557383bc
+	0f1422b22a439a8e6a89032ab25d4e78500c09bf

Diff:

Revision 5 (+968 -1)

Show changes

	setup.py
	rbintegrations/extension.py
	rbintegrations/static/css/travisci/integration-config.less
	rbintegrations/static/images/travisci/icon.png
	rbintegrations/static/images/travisci/icon@2x.png
	rbintegrations/static/js/travisci/integrationConfig.es6.js
	rbintegrations/travisci/__init__.py
	rbintegrations/travisci/api.py
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbintegrations/travisci/forms.py (Diff revision 5)
The issue has been dropped. Show all issues
```
W503 line break before binary operator
```
rbintegrations/travisci/integration.py (Diff revision 5)
The issue has been dropped. Show all issues
```
E127 continuation line over-indented for visual indent
```
rbintegrations/travisci/views.py (Diff revision 5)
The issue has been dropped. Show all issues
```
E501 line too long (83 > 79 characters)
```

The issue has been resolved. Show all issues

Small typo in the example generated configuration. The comma shouldn't be there after the DJANGO_VERSION=1.6.11.

rbintegrations/extension.py (Diff revision 5)

The issue has been dropped. Show all issues

Can we use travis-ci instead of travisci?

david June 28, 2017, 11 a.m.

Given that a lot of the other filenames need to be "travisci" (python modules, etc), I'd rather keep them all consistent.

rbintegrations/static/js/travisci/integrationConfig.es6.js (Diff revision 5)

The issue has been resolved. Show all issues

You can do:

$server.setVisible($endpoint.val() === 'E');

rbintegrations/travisci/api.py (Diff revision 5)

The issue has been resolved. Show all issues

Maybe condense these to be two lines?

u = urlopen(URLRequest(...))
return json.loads(u.read())

rbintegrations/travisci/api.py (Diff revision 5)
The issue has been resolved. Show all issues
```
dict?
```
rbintegrations/travisci/forms.py (Diff revision 5)
The issue has been resolved. Show all issues
```
reviewboard.reviews.models.review_request.ReviewRequest
```

rbintegrations/travisci/forms.py (Diff revision 5)

The issue has been resolved. Show all issues

Maybe we can use the existing list and perform a - [ReviewRequestRepositoryTypeChoice] (or whatever)?

rbintegrations/travisci/forms.py (Diff revision 5)

The issue has been resolved. Show all issues

and on the previous line.

david June 28, 2017, 11 a.m.

Normally yes but that would mean wrapping things in a much more ugly way. This is a lot more readable.

chipx86 July 7, 2017, 2:47 p.m.

We're going to get the error every single time this file goes through review, which is annoying. Let's just pull out stuff as variables and shrink the conditional.

rbintegrations/travisci/forms.py (Diff revision 5)
The issue has been resolved. Show all issues
```
We should probably use six.text_type(e) instead of e directly.
Same below.
```

rbintegrations/travisci/forms.py (Diff revision 5)

The issue has been resolved. Show all issues

Can simplify this a bit:

if warning['key']:
    ...

    message = ...
else:
    message = warning['message']

self._errors['travis_yml'] = self.error_class([message])

rbintegrations/travisci/forms.py (Diff revision 5)

The issue has been resolved. Show all issues

Can we log with request=self.request on all these logging statements, so we'll have user/Local Site context?

rbintegrations/travisci/forms.py (Diff revision 5)

The issue has been resolved. Show all issues

repository's

Let's also just use double quotes for this string instead of escaping the apostrophes.

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

The issue has been resolved. Show all issues

Let's pull out the repository into a variable. It'll prevent the Review Bot complaint, since it should be able to wrap, and we access the repository further below anyway.

rbintegrations/travisci/integration.py (Diff revision 5)
The issue has been resolved. Show all issues
```
We can include this in the function.
```

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

The issue has been resolved. Show all issues

Can these be:

notifications = travis_config.get('notifications') or {}


?

rbintegrations/travisci/integration.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Can use += [ ... ]
```

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

The issue has been resolved. Show all issues

We're going to need to check for IntegrityErrors, in case another process beat this one to the punch on creating the user.

rbintegrations/travisci/views.py (Diff revision 5)
The issue has been resolved. Show all issues
```
*args
```
rbintegrations/travisci/views.py (Diff revision 5)
The issue has been resolved. Show all issues
```
**kwargs
```
rbintegrations/travisci/views.py (Diff revision 5)
The issue has been resolved. Show all issues
```
You can wrap after the . anywhere now.
```
rbintegrations/travisci/views.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This will crash if HTTP_SIGNATURE isn't present.
```
rbintegrations/travisci/views.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Should be bullet-proof this? Log an error if we get a KeyError?
```
rbintegrations/travisci/views.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Does this raise an exception if any part of this fails?
```
rbintegrations/travisci/views.py (Diff revision 5)
The issue has been resolved. Show all issues
```
We should bullet-proof this as well.
```

Description:

		This change adds a new integration which allows people to build prospective
		changes via Travis CI.

		The Travis CI API allows triggering a build in a way that overrides the config
		from the repository's `.travis.yml` file. We use this to send a new
		configuration which knows how to apply the patch and report the results back to
		Review Board.

		The configuration for the integration allows users to set the conditions
		normally, with the exception that the choices for the "Repository" condition
		are limited to only show GitHub repositories (since Travis CI doesn't work with
		anything else). It also requires selecting which Travis endpoint to use (either
		travis-ci.org for open-source projects, travis-ci.com for private projects, or
		an enterprise server with a custom domain), along with providing an API token.

		Currently, builds show up under the main "Branches" section of the repository
		in the Travis CI user interface. Because this can pollute things (for example,
		showing that the `master` branch is broken because a build of a review request
		failed), for now I'm recommending that people create a fake `review-requests`
		branch on their GitHub remote and then set that branch name in the integration
		config. This isn't perfect, but it at least keeps review requests builds
		separate. I'm starting to talk to the Travis CI folks to see if we can get some
		more control through the API, perhaps even letting us list these builds in the
		"Pull Requests" section instead of the "Branches" section.

		The modifications made to the user-entered configuration involve changing the
		`script` section to insert a few commands to check out the correct parent
		revision and apply the patch, and modifications to the `notifications` section
		to disable e-mail notification and set up a webhook to receive results. For
		example, if the build config was set to the following:



		`language: python`
		python:
		- 2.7
		env:
		- DJANGO_VERSION=1.6.11
		- DJANGO_VERSION=1.8.2
		install:
		- python setup.py develop
		- pip install -r dev-requirements.txt
		script:
		- python ./tests/runtests.py




		Then the resulting configuration that would be sent to Travis would look
		something like this:



		`language: python`
		python:
		- 2.7
		env:
		global:
		- REVIEWBOARD_STATUS_UPDATE_ID=71
		- REVIEWBOARD_TRAVIS_INTEGRATION_CONFIG_ID=4
		matrix:
~		- DJANGO_VERSION=1.6.11,
	~	- DJANGO_VERSION=1.6.11
		- DJANGO_VERSION=1.8.2
		install:
		- python setup.py develop
		- pip install -r dev-requirements.txt
		merge_mode: replace
		notifications:
		email: false
		webhooks:
		on_start: always
		urls:
		- http://reviewboard.example.com/rbintegrations/travis-ci/webhook/
		script:
		- git fetch --unshallow origin
		- git checkout 5ec2912cd1cad96a3a20ea5857e9ed2d1de0d465
		- echo <big base64 blob> \| base64 --decode \| patch -p1
		- python ./tests/runtests.py

-
-
-
-		Documentation and unit tests will be forthcoming in future changes.

Commit:

-	0f1422b22a439a8e6a89032ab25d4e78500c09bf
+	d2840643f6e929a6da5872400244fe9ebeb3e5e7

Diff:

Revision 6 (+970 -1)

Show changes

	setup.py
	rbintegrations/extension.py
	rbintegrations/static/css/travisci/integration-config.less
	rbintegrations/static/images/travisci/icon.png
	rbintegrations/static/images/travisci/icon@2x.png
	rbintegrations/static/js/travisci/integrationConfig.es6.js
	rbintegrations/travisci/__init__.py
	rbintegrations/travisci/api.py
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbintegrations/travisci/forms.py (Diff revision 6)
The issue has been dropped. Show all issues
```
W503 line break before binary operator
```
rbintegrations/travisci/views.py (Diff revision 6)
The issue has been resolved. Show all issues
```
E501 line too long (81 > 79 characters)
```
rbintegrations/travisci/views.py (Diff revision 6)
The issue has been dropped. Show all issues
```
E501 line too long (83 > 79 characters)
```
rbintegrations/travisci/views.py (Diff revision 6)
The issue has been resolved. Show all issues
```
E122 continuation line missing indentation or outdented
```

Commit:

-	d2840643f6e929a6da5872400244fe9ebeb3e5e7
+	31e510562cf021a7d67cb92dadfc23adad7ccde2

Diff:

Revision 7 (+971 -1)

Show changes

	setup.py
	rbintegrations/extension.py
	rbintegrations/static/css/travisci/integration-config.less
	rbintegrations/static/images/travisci/icon.png
	rbintegrations/static/images/travisci/icon@2x.png
	rbintegrations/static/js/travisci/integrationConfig.es6.js
	rbintegrations/travisci/__init__.py
	rbintegrations/travisci/api.py
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbintegrations/travisci/forms.py (Diff revision 7)
The issue has been dropped. Show all issues
```
W503 line break before binary operator
```
rbintegrations/travisci/views.py (Diff revision 7)
The issue has been dropped. Show all issues
```
E501 line too long (83 > 79 characters)
```

rbintegrations/static/js/travisci/integrationConfig.es6.js (Diff revision 7)
The issue has been resolved. Show all issues
```
You can pass the method in directory. There's no this being used.
```

rbintegrations/travisci/api.py (Diff revision 7)

The issue has been dropped. Show all issues

We should guard against the request failing or the data not being parsable. (Proxy servers may get in the way and spit out garbage, for instance.)
What we do in GitHub and other places is we have a function that does the request and returns JSON data, and it checks everything. We should maybe have _make_request do that (optionally, if it's being used for non-JSON requests as well).

david July 8, 2017, 1:28 p.m.

The caller has error handling for all of this. I'll clarify that this can raise other errors.

rbintegrations/travisci/forms.py (Diff revision 7)
The issue has been resolved. Show all issues
```
"GitHub"
```

rbintegrations/travisci/forms.py (Diff revision 7)

The issue has been resolved. Show all issues

Can you wrap these in parens so it's a little more visually clear what's happening?
Since this is Python 2.7, we can also use {...} instead of set([...]}.

rbintegrations/travisci/views.py (Diff revision 7)

The issue has been resolved. Show all issues

Sine this line's too long, and we dig into cryptography.hazmat.primitives a bunch, let's just pull primitives out.

rbintegrations/travisci/forms.py (Diff revision 7)
The issue has been resolved. Show all issues
```
GitHub
```

rbintegrations/travisci/integration.py (Diff revision 7)

The issue has been dropped. Show all issues

Should this be marked for translation?

david July 8, 2017, 1:28 p.m.

We don't currently translate the descriptions for the other integrations. Maybe we should, but if so we'll probably want to audit the whole codebase for i18n-related things.

rbintegrations/travisci/integration.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Do we want to limit the scope of this token?
```
1. david July 8, 2017, 1:28 p.m.
  We don't actually need the API token for travis. I'll get rid of this.

Commit:

-	31e510562cf021a7d67cb92dadfc23adad7ccde2
+	958654befc4863eb1613d1959f8120f08f3ea036

Diff:

Revision 8 (+973 -1)

Show changes

	setup.py
	rbintegrations/extension.py
	rbintegrations/static/css/travisci/integration-config.less
	rbintegrations/static/images/travisci/icon.png
	rbintegrations/static/images/travisci/icon@2x.png
	rbintegrations/static/js/travisci/integrationConfig.es6.js
	rbintegrations/travisci/__init__.py
	rbintegrations/travisci/api.py
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbintegrations/travisci/forms.py (Diff revision 8)
The issue has been resolved. Show all issues
```
W503 line break before binary operator
```

Commit:

-	958654befc4863eb1613d1959f8120f08f3ea036
+	c032765c380055b556c547efe348ec0b2020432b

Diff:

Revision 9 (+973 -1)

Show changes

	setup.py
	rbintegrations/extension.py
	rbintegrations/static/css/travisci/integration-config.less
	rbintegrations/static/images/travisci/icon.png
	rbintegrations/static/images/travisci/icon@2x.png
	rbintegrations/static/js/travisci/integrationConfig.es6.js
	rbintegrations/travisci/__init__.py
	rbintegrations/travisci/api.py
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

Barring the UI issues below, this all looks good to me.

Integration configuration (Revision 1)

The issue has been dropped. Show all issues

This help text should probs be aligned with the description above.

david July 14, 2017, 3:21 p.m.

This is style stuff in RB itself. Shouldn't be fixed as part of this change.

Integration configuration (Revision 1)
The issue has been dropped. Show all issues
```
Alignment here is off.
```
1. david July 14, 2017, 3:21 p.m.
  Same with this.

Integration configuration (Revision 1)

The issue has been resolved. Show all issues

We definitely should have a warning that they should omit secret environment variables, because people will be able to print out the environment in the log with a malicious patch.

Status: Closed (submitted)

Change Summary:

Pushed to master (63b89ca)