Add integration for CircleCI.

Review Request #8951 - Created May 19, 2017 and submitted

David Trowbridge
rbintegrations
master
92bed80...
rbintegrations

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.

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

  • 0
  • 0
  • 21
  • 3
  • 24
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

David Trowbridge
David Trowbridge
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

David Trowbridge
Christian Hammond
  1. 
      
  2. 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.

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

    Does CircleCI support GitHub Enterprise?

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

    Why not put this in the argument list?

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

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

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

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

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

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

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

    How does this user work when using Local Sites?

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

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

    Blank line before the comment.

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

  11. rbintegrations/circleci/tests.py (Diff revision 4)
     
     

    We need unit tests handling Local Site scenarios.

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

    **kwargs

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

    Let's dump the payload so we have something to go off of for debugging.

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

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

    Display ID + Local Site name

  17. 
      
David Trowbridge
Christian Hammond
  1. 
      
  2. rbintegrations/circleci/forms.py (Diff revision 5)
     
     

    Missing period.

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

    1. Just forgot to delete this line.

  4. rbintegrations/circleci/tests.py (Diff revision 5)
     
     

    Why _response? Not _Response?

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

  6. 
      
David Trowbridge
Barret Rennie
  1. This LGTM!

  2. 
      
David Trowbridge
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (25d4cb8)
Loading...