Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prefer https:// for URLs throughout project #4805

Merged
merged 1 commit into from Oct 1, 2018
Merged

Prefer https:// for URLs throughout project #4805

merged 1 commit into from Oct 1, 2018

Conversation

jdufresne
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Sep 23, 2018

Codecov Report

Merging #4805 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4805   +/-   ##
=======================================
  Coverage   66.77%   66.77%           
=======================================
  Files          15       15           
  Lines        1568     1568           
=======================================
  Hits         1047     1047           
  Misses        521      521
Impacted Files Coverage Δ
requests/models.py 77.35% <ø> (ø) ⬆️
requests/api.py 36.36% <ø> (ø) ⬆️
requests/sessions.py 76.34% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 207e83f...839af77. Read the comment docs.

@kennethreitz
Copy link
Contributor

Did you verify all of these work?

@jdufresne
Copy link
Contributor Author

Hi Kenneth,

I've done this same change on a number of different repositories now. I automated this change using the script:

https://github.com/jdufresne/commit-messages/blob/master/scripts/checkhttp.py

At a high level, this script does the following:

  1. Scans all text files for strings that look like a URL starting with http://
  2. Tries fetching the same URL but with https://
  3. Check the final destination is a 200 response with protocol https:// (to avoid redirects back to http://)
  4. If it looks good, replace all occurrences

After running the script, I analyze the results locally using a visual diff tool (I like meld). I undo the changes for tests that look to be intentionally using an http:// URL (requests has a lot of these). I also try to remove en/latest/ paths from readthedocs that didn't previously exist. I manually test a bunch of URLs especially any that look suspicious with a wildly different path. I re-analyze the results using GitHub's diff tool (I find looking at a diff in multiple ways can help emphasize different differences).

Many of the changed URLs share a domain. So after spot checking a few from these domains, we can be confident with the rest.

So, in summary, a script has confirmed that all these changes result in https:// URLs that 200 and several have been manually tested.

Please feel free to re-run the script to verify or do additional manual testing.

Copy link
Member

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a couple examples may be broken. I left comments on the two I had a concern about. Once those are addressed and we get this rebased onto master, I think this is set for merge.

@@ -25,8 +25,8 @@ Let's persist some cookies across requests::

s = requests.Session()

s.get('http://httpbin.org/cookies/set/sessioncookie/123456789')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed?

@@ -69,7 +69,7 @@ If you want to manually add cookies to your session, use the
Sessions can also be used as context managers::

with requests.Session() as s:
s.get('http://httpbin.org/cookies/set/sessioncookie/123456789')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here.

@jdufresne
Copy link
Contributor Author

Thanks for catching those mistakes. Restored and fixed in the latest revision.

Copy link
Member

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I think this looks good. Thanks @jdufresne! ✨

@nateprewitt nateprewitt merged commit ff0c325 into psf:master Oct 1, 2018
This was referenced Mar 9, 2021
This was referenced Mar 15, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants