Summary

Fix GitHub auth token bug using wrong HTTP header capitalization in Python2.7

Review Request #11173 — Created Sept. 17, 2020 and submitted Sept. 24, 2020, 12:23 a.m.

Information

Owner

hailan

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

When trying to add a newly configured repo, it complained about the
GitHub Auth Token missing required permissions.

The underlying issue was that the wrong capitalization was used when
getting the OAuth scopes header for Python2.7.

Testing Done

Did the following with all relevant versions of Python3 and 2:

Ran unit tests and they passed
Manually added a repo with with a personal access token

Commits

Summary	ID	Author
Normalize headers from GitHub for consistency in Python 2 and 3	d71bb40fba95823bd8afc846196986da26191112	HailanXyouknow

Issues

Description	From	Last Updated
The summary doesn't need to contain the branch name (that's already shown in the "Branch" field). We can also be …	david	Sept. 18, 2020, 12:57 p.m.
The testing done is quite comprehensive, and I don't want to discourage that too much, but we can make it …	david	Sept. 18, 2020, 12:59 p.m.
For the description, you can remove the first line. We don't need to duplicate the information from the summary. The …	david	Sept. 18, 2020, 1 p.m.
If we're going to wrap the description at 70 columns, then I think the newline breaks need to happen after …	mike_conley	Sept. 19, 2020, 11:21 a.m.
So I just was running my devserver on Python 3.8, and I seem to be seeing the header with the …	david	Sept. 20, 2020, 9:37 a.m.
Just to check, did you run the run tests on both Python 2.x and Python 3.x? If so, can you …	david	Sept. 22, 2020, 3:31 p.m.
W293 blank line contains whitespace	reviewbot	Sept. 20, 2020, 9:37 a.m.
W291 trailing whitespace	reviewbot	Sept. 20, 2020, 9:37 a.m.
E231 missing whitespace after ','	reviewbot	Sept. 20, 2020, 9:37 a.m.
E231 missing whitespace after ':'	reviewbot	Sept. 20, 2020, 9:37 a.m.
Let's flesh this out a bit. Maybe something like: urllib returns different capitalization for header names depending on whether we're …	david	Sept. 22, 2020, 3:31 p.m.

flake8 passed.

JSHint passed.

Description:

		Fix GitHub Auth token bug for when configuring a repo (release-4.0.x)
	+
	+	When trying to add a newly configured repo, it complained about the GitHub Access Token to not have all the required permissions. I've updated the name of the key we want to read from the HTTP request response.

Groups:

+	students

The code itself looks good. I do have a few suggestions for how we can improve the summary/description/testing done:

The issue has been resolved. Show all issues

The summary doesn't need to contain the branch name (that's already shown in the "Branch" field).

We can also be a bit more specific here. Something like "Fix GitHub auth token bug using wrong HTTP header capitalization"

The issue has been resolved. Show all issues

The testing done is quite comprehensive, and I don't want to discourage that too much, but we can make it more concise. I usually do something like:

Ran unit tests.
Was able to manually add a GitHub repository using a personal auth token.

The issue has been resolved. Show all issues

For the description, you can remove the first line. We don't need to duplicate the information from the summary.

The description is pretty good about explaining the user-visible appearance of the bug, but let's also explain the underlying cause (We were correctly fetching the token, but then looking for the wrong capitalization in the HTTP headers).

Let's also wrap the description to 70 columns, which makes it easier to read, especially when it gets sucked into the commit message when we land it.

Summary:

-	Fix GitHub Auth token bug for when configuring a repo (release-4.0.x)
+	Fix GitHub auth token bug using wrong HTTP header capitalization

Description:

~		Fix GitHub Auth token bug for when configuring a repo (release-4.0.x)
~
~		When trying to add a newly configured repo, it complained about the GitHub Access Token to not have all the required permissions. I've updated the name of the key we want to read from the HTTP request response.
	~	When trying to add a newly configured repo,
	~	it complained about the GitHub Auth Token missing required permissions
	~	Underlying issue: We were fetching the correct token,
	+	but it was looking for the wrong capitalization.

Testing Done:

~		Commands I've ran:
~		./tests/runtests.py reviewboard.hostingsvcs.tests.test_github:GitHubTests
	~	Ran unit tests and they passed
	~	Manually added a repo with a personal access token and it worked
-		./tests/runtests.py
-
-		OUTPUT FROM LAST COMMAND:
-
-		Ran 5483 tests in 578.370s
-
-		OK (SKIP=89)
-
-		Also I have tested it manually:
-

Commits:

	Summary	ID	Author
	fix bug with GitHub Auth tokens when configuring a repo (release-4.0.x)	3b470b8c48f788b886134c63cc0758a50e3aa5e3	HailanXyouknow
	Fix GitHub auth token bug using wrong HTTP header capitalization	f0f3172588ad0dfc9c14f3aa20a7a09cf7bcb67d	HailanXyouknow

Diff:

Revision 2 (+6 -6)

Show changes

	reviewboard/hostingsvcs/github.py
	reviewboard/hostingsvcs/tests/test_github.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

		When trying to add a newly configured repo,
~		it complained about the GitHub Auth Token missing required permissions
	~	it complained about the GitHub Auth Token missing required permissions.
		Underlying issue: We were fetching the correct token,
		but it was looking for the wrong capitalization.

The issue has been resolved. Show all issues

If we're going to wrap the description at 70 columns, then I think the newline breaks need to happen after "about the".

Maybe cut and paste the description into your editor to make sure it's formatted correctly. Your editor should probably have some kind of column counter visible to tell you where your cursor is - or might even have a built in visual ruler.

I also think "Underlying issue:" as a section isn't really needed. We can turn this into a sentence, like,

"The underlying issue was that we weren't using the right capitalization when getting the OAuth scopes header."

Something like that, anyhow.

hailan Sept. 19, 2020, 11:21 a.m.

Ah, yes! The editor does have a column counter, I just noticed that!
Thank you :D

Description:

~		When trying to add a newly configured repo,
~		it complained about the GitHub Auth Token missing required permissions.
~		Underlying issue: We were fetching the correct token,
~		but it was looking for the wrong capitalization.
	~	When trying to add a newly configured repo, it complained about the
	~	GitHub Auth Token missing required permissions.
	~
	~	The underlying issue was that the wrong capitalization was used when
	+	getting the OAuth scopes header.

Ship it!

```
Looks great, thank you!
```

```
Ship It!
```

The issue has been resolved. Show all issues

So I just was running my devserver on Python 3.8, and I seem to be seeing the header with the older capitalization. Can you test that this works when running on that?

If not, we probably need to normalize header capitalization throughout.

hailan Sept. 19, 2020, 1:47 p.m.

Oops, was just running it like './contrib/internal/devserver.py' without any python version attached.
Will be checking it against all Python versions next time.
Yes, I will be normalizing it. Thanks David :)

Change Summary:

Changes:

Added a line to normalize keys from header
Deleted the changes to hostingsvcs/tests/test_github.py to test
the normalization of the keys in the header

Summary:

-	Fix GitHub auth token bug using wrong HTTP header capitalization
+	Fix GitHub auth token bug using wrong HTTP header capitalization in Python2.7

Description:

		When trying to add a newly configured repo, it complained about the
		GitHub Auth Token missing required permissions.

		The underlying issue was that the wrong capitalization was used when
~		getting the OAuth scopes header.
	~	getting the OAuth scopes header for Python2.7.

Testing Done:

		Ran unit tests and they passed
~		Manually added a repo with a personal access token and it worked
	~	Manually added a repo with Python3.8, 3.7, 3.6 and 2.7 with a personal access token and it worked

Commits:

	Summary	ID	Author
	Fix GitHub auth token bug using wrong HTTP header capitalization	f0f3172588ad0dfc9c14f3aa20a7a09cf7bcb67d	HailanXyouknow
	Fix GitHub auth token bug using wrong HTTP header capitalization in Python2	76ca7e44375f653110b8fa6824db65856cb78aa7	HailanXyouknow

Diff:

Revision 3 (+8 -2)

Show changes

reviewboard/hostingsvcs/github.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/hostingsvcs/github.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W293 blank line contains whitespace
```
reviewboard/hostingsvcs/github.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```
reviewboard/hostingsvcs/github.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E231 missing whitespace after ','
```
reviewboard/hostingsvcs/github.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E231 missing whitespace after ':'
```

Commits:

	Summary	ID	Author
	Fix GitHub auth token bug using wrong HTTP header capitalization in Python2	76ca7e44375f653110b8fa6824db65856cb78aa7	HailanXyouknow
	Fix GitHub auth token bug using wrong HTTP header capitalization in Python2	5f75150837dd0ea6e9a19f7388d05282e936913f	HailanXyouknow

Diff:

Revision 4 (+8 -2)

Show changes

reviewboard/hostingsvcs/github.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Just to check, did you run the run tests on both Python 2.x and Python 3.x? If so, can you make that clear in your testing done section?

reviewboard/hostingsvcs/github.py (Diff revision 4)

The issue has been resolved. Show all issues

Let's flesh this out a bit. Maybe something like:

urllib returns different capitalization for header names depending on whether we're on Python 2 or Python 3. Normalize everything is in lower case for consistency.

Note that there's also a typo here (oath -> oauth), in case you keep any of your prior comment's wording.

Testing Done:

~		Ran unit tests and they passed
~		Manually added a repo with Python3.8, 3.7, 3.6 and 2.7 with a personal access token and it worked
	~	Did the following with all relevant versions of Python3 and 2:
	~
	+	Ran unit tests and they passed
	+	Manually added a repo with with a personal access token

Commits:

	Summary	ID	Author
	Fix GitHub auth token bug using wrong HTTP header capitalization in Python2	5f75150837dd0ea6e9a19f7388d05282e936913f	HailanXyouknow
	Normalize headers from GitHub for consistency in Python 2 and 3	d71bb40fba95823bd8afc846196986da26191112	HailanXyouknow

Diff:

Revision 5 (+12 -2)

Show changes

reviewboard/hostingsvcs/github.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (b6923f4)

Fix GitHub auth token bug using wrong HTTP header capitalization in Python2.7

Information

Reviewers

Description:

Groups:

Summary:

Description:

Testing Done:

OUTPUT FROM LAST COMMAND:

Commits:

Diff:

Checks run (2 succeeded)

Description:

Description:

Change Summary:

Summary:

Description:

Testing Done:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (2 succeeded)

Testing Done:

Commits:

Diff:

Checks run (2 succeeded)

Status: Closed (submitted)

Change Summary: