- Download Diff

Summary:

Fix CircleCI build not running on personal GitHub repositories.

Review Request #11190 — Created Sept. 19, 2020 and submitted Dec. 13, 2020, 3:41 a.m.

Information
Owner:	MarcusBoay
Repository:	rbintegrations
Branch:	master
Bugs:
Depends On:
Reviewers
Groups:	rbintegrations
People:

Description

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.

Testing Done

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.

Commits

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.

Screenshots

Issues

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 …	chipx86	Sept. 20, 2020, 9:42 a.m.
We generally avoid shortening words, because not everybody's coming in with the same level of knowledge. "repositories" would be better …	chipx86	Sept. 20, 2020, 9:42 a.m.
Change descriptions should wrap at around 75 characters. This goes for the description and testing.	chipx86	Sept. 20, 2020, 9:42 a.m.
The description is helpful, but it doesn't really say what the problem really was and how it was fixed. This …	chipx86	Sept. 20, 2020, 9:46 a.m.
Your testing mentions "manual testing," but doesn't mention anything about unit tests. Since you caught a bug manually that hadn't …	chipx86	Sept. 23, 2020, 10:24 p.m.
Doing a quick grep through the codebase, I can see that the Travis CI support has the same problem. Maybe …	chipx86	Sept. 23, 2020, 11:30 p.m.
W293 blank line contains whitespace	reviewbot	Oct. 8, 2020, 10:20 p.m.
W292 no newline at end of file	reviewbot	Oct. 8, 2020, 10:20 p.m.
The type name here should be unicode, rather than string.	david	Oct. 12, 2020, 2:33 p.m.
Type here should be unicode rather than str.	david	Oct. 12, 2020, 2:33 p.m.
json isn't a type in Python. This is returning dict.	david	Oct. 12, 2020, 2:33 p.m.

flake8 passed.

JSHint passed.

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

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.

Show all issues

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

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.

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

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?

MarcusBoay Sept. 23, 2020, 11:30 p.m.

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

Summary:

-	Fix CI build not running on personal GitHub repos.
+	Fix CircleCI build not running on personal GitHub repositories.

Description:

~		Fix CI build not running on personal GitHub repos.
	~	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.
	~	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`.

Testing Done:

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

Testing Done:

~		Manual testing. A personal public and private repository was set up and
	~	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.

Commits:

	Summary
-	Fix CI build not running on personal GitHub repos.
+	Fix CI build not running on personal GitHub repos.
+	Add CircleCI different GitHub repository plan tests.

Diff:

Revision 2 (+292 -16)

Show changes

	rbintegrations/circleci/integration.py
	rbintegrations/circleci/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commits:

	Summary
-	Fix CI build not running on personal GitHub repos.
-	Add CircleCI different GitHub repository plan tests.
+	Fix CI build not running on personal GitHub repos.
+	Add CircleCI different GitHub repository plan tests.

Diff:

Revision 3 (+298 -16)

Show changes

	rbintegrations/circleci/integration.py
	rbintegrations/circleci/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

		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 for the unit tests to pass.

Change Summary:

Update testing description with RB3 and RB4

Description:

		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 for the unit tests to pass.
	~	This patch depends on /r/11198 and /r/11204 for the unit tests to pass.

Testing Done:

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

Change Summary:

Add missing documentation

Commits:

	Summary
-	Fix CI build not running on personal GitHub repos.
-	Add CircleCI different GitHub repository plan tests.
+	Fix CI build not running on personal GitHub repos.
+	Add CircleCI different GitHub repository plan unit tests.

Diff:

Revision 4 (+310 -16)

Show changes

	rbintegrations/circleci/integration.py
	rbintegrations/circleci/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

	rbintegrations/circleci/integration.py
	rbintegrations/circleci/tests.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbintegrations/circleci/tests.py (Diff revision 5)
Show all issues
```
W293 blank line contains whitespace
```
rbintegrations/circleci/tests.py (Diff revision 5)
Show all issues
```
W292 no newline at end of file
```

Change Summary:

Resolve flake8 issue

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

Diff:

Revision 6 (+331 -151)

Show changes

	rbintegrations/circleci/integration.py
	rbintegrations/circleci/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Just a few simple comments about types in docstrings.

MarcusBoay Oct. 12, 2020, 2:33 p.m.

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

rbintegrations/circleci/integration.py (Diff revision 6)
Show all issues
```
The type name here should be unicode, rather than string.
```
rbintegrations/circleci/tests.py (Diff revision 6)
Show all issues
```
Type here should be unicode rather than str.
```
rbintegrations/circleci/tests.py (Diff revision 6)
Show all issues
```
json isn't a type in Python. This is returning dict.
```

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

	rbintegrations/circleci/integration.py
	rbintegrations/circleci/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (4a5a3c7)

You have a pending review.

Review Board 5.0.2 alpha 0 (dev)

Fix CircleCI build not running on personal GitHub repositories.

Summary:

Description:

Testing Done:

Testing Done:

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 succeeded)

Description:

Change Summary:

Description:

Testing Done:

Change Summary:

Commits:

Diff:

Checks run (2 succeeded)

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 succeeded)

Status: Closed (submitted)

Change Summary: