Add an integration for Travis CI.

Review Request #8921 - Created May 5, 2017 and submitted

David Trowbridge
rbintegrations
master
c032765...
rbintegrations

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

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.
Loading file attachments...

  • 0
  • 0
  • 58
  • 25
  • 83
Description From Last Updated
Checks run (2 failed, 1 failed with error)
JSHint failed.
PEP8 Style Checker internal error.
Pyflakes failed.

JSHint

Pyflakes

David Trowbridge
Barret Rennie
  1. 
      
  2. Any reason you don't use an arrow func here?

    1. 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.

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

    Module docstring

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

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

    1. 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.

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

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

    "HTTP GET"

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

    Should be indented.

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

    Needs to be qualified.

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

    Should be indented.

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

    Needs to be qualified.

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

    Needs to be qualified.

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

    Needs to be qualified.

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

    Should be indented.

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

    Needs to be qualified.

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

    Should be indented.

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

    Module level docstring.

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

    Don't we usually have these at top of funcs?

    1. No? I prefer to import things adjacent to where they're used.

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

    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.

    1. 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.

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

    Module level docstring.

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

    Can we distribute these?

    1. Yep, so long as we follow their normal branding guidelines.

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

    Rather, review requests that are attachment-only.

    1. 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.

    2. 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.

    3. I made this comment better in the latest diff.

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

    This isn't Review Bot.

  22. rbintegrations/travisci/views.py (Diff revision 2)
     
     

    Module level docstring.

  23. rbintegrations/travisci/views.py (Diff revision 2)
     
     

    No blank line here.

  24. rbintegrations/travisci/views.py (Diff revision 2)
     
     
     

    Blank line between these.

  25. 
      
David Trowbridge
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Barret Rennie
  1. 
      
  2. rbintegrations/travisci/forms.py (Diff revision 3)
     
     

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

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

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

  3. 
      
David Trowbridge
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

David Trowbridge
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

  3. rbintegrations/extension.py (Diff revision 5)
     
     

    Can we use travis-ci instead of travisci?

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

  4. You can do:

    $server.setVisible($endpoint.val() === 'E');
    
  5. rbintegrations/travisci/api.py (Diff revision 5)
     
     
     
     
     

    Maybe condense these to be two lines?

    u = urlopen(URLRequest(...))
    return json.loads(u.read())
    
  6. rbintegrations/travisci/api.py (Diff revision 5)
     
     
     
  7. rbintegrations/travisci/forms.py (Diff revision 5)
     
     

    reviewboard.reviews.models.review_request.ReviewRequest

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

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

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

    and on the previous line.

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

    2. 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.

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

    We should probably use six.text_type(e) instead of e directly.

    Same below.

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

    Can simplify this a bit:

    if warning['key']:
        ...
    
        message = ...
    else:
        message = warning['message']
    
    self._errors['travis_yml'] = self.error_class([message])
    
  12. rbintegrations/travisci/forms.py (Diff revision 5)
     
     
     
     

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

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

    repository's

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

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

    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.

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

    We can include this in the function.

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

    Can these be:

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

    ?

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

    Can use += [ ... ]

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

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

  19. rbintegrations/travisci/views.py (Diff revision 5)
     
     
  20. rbintegrations/travisci/views.py (Diff revision 5)
     
     

    **kwargs

  21. rbintegrations/travisci/views.py (Diff revision 5)
     
     

    You can wrap after the . anywhere now.

  22. rbintegrations/travisci/views.py (Diff revision 5)
     
     

    This will crash if HTTP_SIGNATURE isn't present.

  23. rbintegrations/travisci/views.py (Diff revision 5)
     
     

    Should be bullet-proof this? Log an error if we get a KeyError?

  24. rbintegrations/travisci/views.py (Diff revision 5)
     
     
     

    Does this raise an exception if any part of this fails?

  25. rbintegrations/travisci/views.py (Diff revision 5)
     
     

    We should bullet-proof this as well.

  26. 
      
David Trowbridge
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

David Trowbridge
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Christian Hammond
  1. 
      
  2. You can pass the method in directory. There's no this being used.

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

    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).

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

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

    "GitHub"

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

    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([...]}.

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

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

  7. 
      
Barret Rennie
  1. 
      
  2. rbintegrations/travisci/forms.py (Diff revision 7)
     
     
  3. rbintegrations/travisci/integration.py (Diff revision 7)
     
     

    Should this be marked for translation?

    1. 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.

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

    Do we want to limit the scope of this token?

    1. We don't actually need the API token for travis. I'll get rid of this.

  5. 
      
David Trowbridge
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

David Trowbridge
Barret Rennie
  1. Barring the UI issues below, this all looks good to me.

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

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

  3. 
      
Barret Rennie
  1. 
      
  2. 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.

  3. 
      
David Trowbridge
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (63b89ca)
Loading...