Fix CircleCI build not running on personal GitHub repositories.

Review Request #11190 — Created Sept. 19, 2020 and submitted

Information

rbintegrations
master

Reviewers

Fix CircleCI build not running on personal GitHub repositories.

CircleCI pipelines were not running for personal GitHub repositories when
a review request is made because the GitHub username was being extracted
from the wrong place.

RBIntegrations was pulling the wrong key from extra_data and it's been
updated to grab the associated HostingService's username.

This patch depends on /r/11198 and /r/11204 for the unit tests to pass.

Manual testing: A personal public and private repository was set up and
integrated with CircleCI. They are confirmed to be running without using
Review Board. Then, a review request was made for both the repositories.
Using the CircleCI web UI, the pipelines were triggered.

Unit tests were created to verify that the username is being pulled
from the correct place for the 4 different GitHub repository plans.
These were ran on Review Board 3.0 and Review Board 4.0 and their
supported versions of Python.

Summary ID
Fix CircleCI not running on personal GitHub repos.
00999ad311dbeb178fa70a100da3d5cd265cc44d
Add CircleCI different GitHub repository plan unit tests.
e788898d1f3a7f977a9b57d3504a63f6faa705df
Refactor CircleCI tests to reduce duplication.
7a08a8b7e2a3e2622112a3d95484174e5f0cdc04
Fix types in CircleCI docstrings to Python types.
0ebb174f668ab7352f4c27454a78f1720c76537a
Description From Last Updated

The change summary should make it more clear which CI we're talking about. There's 3 in rbintegrations. It'd be best …

chipx86chipx86

We generally avoid shortening words, because not everybody's coming in with the same level of knowledge. "repositories" would be better …

chipx86chipx86

Change descriptions should wrap at around 75 characters. This goes for the description and testing.

chipx86chipx86

The description is helpful, but it doesn't really say what the problem really was and how it was fixed. This …

chipx86chipx86

Your testing mentions "manual testing," but doesn't mention anything about unit tests. Since you caught a bug manually that hadn't …

chipx86chipx86

Doing a quick grep through the codebase, I can see that the Travis CI support has the same problem. Maybe …

chipx86chipx86

W293 blank line contains whitespace

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

The type name here should be unicode, rather than string.

daviddavid

Type here should be unicode rather than str.

daviddavid

json isn't a type in Python. This is returning dict.

daviddavid
chipx86
  1. 
      
  2. Show all issues

    The change summary should make it more clear which CI we're talking about. There's 3 in rbintegrations. It'd be best to specifically say "CircleCI".

  3. Show all issues

    We generally avoid shortening words, because not everybody's coming in with the same level of knowledge. "repositories" would be better than "repos" in the summary/description.

  4. Show all issues

    Change descriptions should wrap at around 75 characters. This goes for the description and testing.

  5. Show all issues

    The description is helpful, but it doesn't really say what the problem really was and how it was fixed. This is good information to have so I don't have to dig into code.

    You don't need to go into every little detail, but saying that it was pulling out the wrong key from extra_data and it's been updated to grab the associated HostingServiceAccount's username would be good.

  6. Show all issues

    Your testing mentions "manual testing," but doesn't mention anything about unit tests. Since you caught a bug manually that hadn't been noticed in tests, it's the right time to add some tests to make sure that all of those types of hosting setups do what they're expected to do. One test per setup.

    This is going to be longer than the fix, but since unit tests will be a big part of what you end up doing with any new code you write, now's a great time to learn the tricks and procedures for writing them :)

  7. Show all issues

    Doing a quick grep through the codebase, I can see that the Travis CI support has the same problem. Maybe this change should focus on fixing both?

    1. I'm running into authorization errors for the TravisCI tests. Let's make this a separate patch so that we can land this fix.

  8. 
      
MarcusBoay
MarcusBoay
MarcusBoay
MarcusBoay
MarcusBoay
MarcusBoay
MarcusBoay
MarcusBoay
Review request changed
Change Summary:

Refactor code to reduce duplication

Commits:
Summary ID
Fix CI build not running on personal GitHub repos.
2db84fc46bff4ab8515381d6fb95b49e10221cd5
Add CircleCI different GitHub repository plan unit tests.
ce02f30ca1d14bba62a866a287b44b9cd322660d
Fix CI build not running on personal GitHub repos.
2db84fc46bff4ab8515381d6fb95b49e10221cd5
Add CircleCI different GitHub repository plan unit tests.
ce02f30ca1d14bba62a866a287b44b9cd322660d
Refactor CircleCI tests to reduce duplication.
d33504c8bbe0da9e5d7a2d45f3306919dc83761b

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

MarcusBoay
david
  1. Just a few simple comments about types in docstrings.

    1. Thanks! I was confused as to what types I should put into the docstrings.

  2. rbintegrations/circleci/integration.py (Diff revision 6)
     
     
    Show all issues

    The type name here should be unicode, rather than string.

  3. rbintegrations/circleci/tests.py (Diff revision 6)
     
     
    Show all issues

    Type here should be unicode rather than str.

  4. rbintegrations/circleci/tests.py (Diff revision 6)
     
     
    Show all issues

    json isn't a type in Python. This is returning dict.

  5. 
      
MarcusBoay
david
  1. Ship It!
  2. 
      
chipx86
  1. Ship It!
  2. 
      
MarcusBoay
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (4a5a3c7)