Fix CircleCI build not running on personal GitHub repositories.

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

MarcusBoay
rbintegrations
master
rbintegrations

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
Fix CircleCI not running on personal GitHub repos.
Add CircleCI different GitHub repository plan unit tests.
Refactor CircleCI tests to reduce duplication.
Fix types in CircleCI docstrings to Python types.
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. 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. 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. Change descriptions should wrap at around 75 characters. This goes for the description and testing.

  5. 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. 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. 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
-
Fix CI build not running on personal GitHub repos.
-
Add CircleCI different GitHub repository plan unit tests.
+
Fix CI build not running on personal GitHub repos.
+
Add CircleCI different GitHub repository plan unit tests.
+
Refactor CircleCI tests to reduce duplication.

Diff:

Revision 5 (+331 -151)

Show changes

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)
     
     

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

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

    Type here should be unicode rather than str.

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

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

  5. 
      
MarcusBoay
Review request changed

Commits:

Summary
-
Fix CI build not running on personal GitHub repos.
-
Add CircleCI different GitHub repository plan unit tests.
-
Refactor CircleCI tests to reduce duplication.
+
Fix CircleCI not running on personal GitHub repos.
+
Add CircleCI different GitHub repository plan unit tests.
+
Refactor CircleCI tests to reduce duplication.
+
Fix types in CircleCI docstrings to Python types.

Diff:

Revision 7 (+334 -154)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...