Fix CircleCI build not running on personal GitHub repositories.
Review Request #11190 — Created Sept. 19, 2020 and submitted
Information | |
---|---|
MarcusBoay | |
rbintegrations | |
master | |
Reviewers | |
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 associatedHostingService
'susername
.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 |
---|
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 … |
|
|
We generally avoid shortening words, because not everybody's coming in with the same level of knowledge. "repositories" would be better … |
|
|
Change descriptions should wrap at around 75 characters. This goes for the description and testing. |
|
|
The description is helpful, but it doesn't really say what the problem really was and how it was fixed. This … |
|
|
Your testing mentions "manual testing," but doesn't mention anything about unit tests. Since you caught a bug manually that hadn't … |
|
|
Doing a quick grep through the codebase, I can see that the Travis CI support has the same problem. Maybe … |
|
|
W293 blank line contains whitespace |
![]() |
|
W292 no newline at end of file |
![]() |
|
The type name here should be unicode, rather than string. |
|
|
Type here should be unicode rather than str. |
|
|
json isn't a type in Python. This is returning dict. |
|
-
-
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".
-
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.
-
-
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 associatedHostingServiceAccount
'susername
would be good. -
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 :)
-
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?
Summary: |
|
||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||
Testing Done: |
|
Testing Done: |
|
---|
Commits: |
|
||||||||
---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+292 -16) |
Checks run (2 succeeded)
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+298 -16) |
Checks run (2 succeeded)
Description: |
|
---|
Change Summary:
Update testing description with RB3 and RB4
Description: |
|
|||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Change Summary:
Add missing documentation
Commits: |
|
||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+310 -16) |
Checks run (2 succeeded)
Change Summary:
Refactor code to reduce duplication
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+331 -151) |
Checks run (1 failed, 1 succeeded)
flake8
Change Summary:
Resolve flake8 issue
Commits: |
|
||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+331 -151) |
Checks run (2 succeeded)
-
Just a few simple comments about types in docstrings.
-
rbintegrations/circleci/integration.py (Diff revision 6) The type name here should be
unicode
, rather thanstring
. -
-
rbintegrations/circleci/tests.py (Diff revision 6) json
isn't a type in Python. This is returningdict
.
Commits: |
|
||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+334 -154) |