flake8
passed.
JSHint
passed.
Fix GitHub auth token bug using wrong HTTP header capitalization in Python2.7
Review Request #11173 — Created Sept. 17, 2020 and submitted
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.
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
| Summary | ID | Author |
|---|---|---|
| d71bb40fba95823bd8afc846196986da26191112 | HailanXyouknow |
| 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
|
|
|
The testing done is quite comprehensive, and I don't want to discourage that too much, but we can make it … |
david
|
|
|
For the description, you can remove the first line. We don't need to duplicate the information from the summary. The … |
david
|
|
|
If we're going to wrap the description at 70 columns, then I think the newline breaks need to happen after … |
 mike_conley
|
|
|
So I just was running my devserver on Python 3.8, and I seem to be seeing the header with the … |
david
|
|
|
Just to check, did you run the run tests on both Python 2.x and Python 3.x? If so, can you … |
david
|
|
|
W293 blank line contains whitespace |
 reviewbot
|
|
|
W291 trailing whitespace |
reviewbot
|
|
|
E231 missing whitespace after ',' |
reviewbot
|
|
|
E231 missing whitespace after ':' |
reviewbot
|
|
|
Let's flesh this out a bit. Maybe something like: urllib returns different capitalization for header names depending on whether we're … |
david
|
- 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:
-
The code itself looks good. I do have a few suggestions for how we can improve the summary/description/testing done:
-
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 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.
-
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 3b470b8c48f788b886134c63cc0758a50e3aa5e3 HailanXyouknow f0f3172588ad0dfc9c14f3aa20a7a09cf7bcb67d HailanXyouknow
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.
-
-
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.
- 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.
- Change Summary:
-
Changes:
-
Added a line to normalize keys from header
-
Deleted the changes to
hostingsvcs/tests/test_github.pyto test
the normalization of the keys in the header
-
- Summary:
-
Fix GitHub auth token bug using wrong HTTP header capitalizationFix 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 f0f3172588ad0dfc9c14f3aa20a7a09cf7bcb67d HailanXyouknow 76ca7e44375f653110b8fa6824db65856cb78aa7 HailanXyouknow - Diff:
-
Revision 3 (+8 -2)
- Commits:
-
Summary ID Author 76ca7e44375f653110b8fa6824db65856cb78aa7 HailanXyouknow 5f75150837dd0ea6e9a19f7388d05282e936913f HailanXyouknow - Diff:
-
Revision 4 (+8 -2)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
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?
-
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 5f75150837dd0ea6e9a19f7388d05282e936913f HailanXyouknow d71bb40fba95823bd8afc846196986da26191112 HailanXyouknow - Diff:
-
Revision 5 (+12 -2)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
