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

Conversation

@jdufresne
Copy link
Contributor

@jdufresne jdufresne commented Sep 23, 2018

No description provided.

@codecov-io
Copy link

@codecov-io 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.

@kennethreitz42
Copy link
Contributor

@kennethreitz42 kennethreitz42 commented Sep 24, 2018

Did you verify all of these work?

@jdufresne
Copy link
Contributor Author

@jdufresne jdufresne commented Sep 24, 2018

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

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

This comment has been minimized.

@nateprewitt

nateprewitt Sep 30, 2018
Member

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

This comment has been minimized.

@nateprewitt

nateprewitt Sep 30, 2018
Member

Same question here.

@jdufresne jdufresne force-pushed the jdufresne:https branch from 839af77 to a10cf65 Sep 30, 2018
@jdufresne jdufresne force-pushed the jdufresne:https branch from a10cf65 to b0ad249 Sep 30, 2018
@jdufresne
Copy link
Contributor Author

@jdufresne jdufresne commented Sep 30, 2018

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

Copy link
Member

@nateprewitt nateprewitt left a comment

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

@nateprewitt nateprewitt merged commit ff0c325 into psf:master Oct 1, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants