Skip to content

Conversation

@ntouran
Copy link
Contributor

@ntouran ntouran commented Oct 9, 2023

Subject: Apply tls_verify and tls_cacerts config to ImageDownloader

Feature or Bugfix

  • Bugfix

Purpose

  • Apply existing requests configuration to a request where it is needed in some cases.
  • To fully test, you need a self-signed certificate to put remote images on via URL.

Detail

Relates

@ntouran ntouran force-pushed the tls-config-imagedownloader branch from c5c32c1 to 5d69b5f Compare October 9, 2023 22:56
@picnixz
Copy link
Member

picnixz commented Oct 10, 2023

If possible, can you check where other tls_verify and tls_cacerets values are not applied? Like, are there other places where the same issue could happen?

@ntouran
Copy link
Contributor Author

ntouran commented Oct 10, 2023

If possible, can you check where other tls_verify and tls_cacerets values are not applied? Like, are there other places where the same issue could happen?

I looked around to see if there were any more obvious cases at first but will double check. Good idea.

Ok:

$ git grep 'from sphinx.util import' | grep requests
sphinx/builders/linkcheck.py:from sphinx.util import encode_uri, logging, requests
sphinx/ext/intersphinx.py:from sphinx.util import logging, requests
sphinx/transforms/post_transforms/images.py:from sphinx.util import logging, requests
tests/test_build_linkcheck.py:from sphinx.util import requests

The 3 clients of the requests util here are:

  • linkcheck -- yes, does use this config correctly in HEAD and GET requests
  • intersphinx -- yes, does use proper config already
  • ImageDownloader -- now does correct usage applied by this PR

So I think we're good!

@picnixz
Copy link
Member

picnixz commented Oct 11, 2023

Good then! Could you add a CHANGES entry please?

@AA-Turner I think we're good unless you want to wrap small PRs for the day when 7.3.0 is planned to be released.

@ntouran ntouran force-pushed the tls-config-imagedownloader branch from bd3af9e to 0c72934 Compare October 12, 2023 01:33
@ntouran
Copy link
Contributor Author

ntouran commented Oct 12, 2023

Could you add a CHANGES entry please?

Done!

@picnixz
Copy link
Member

picnixz commented Oct 12, 2023

Perfect! In the future, if you want to contribute again, don't force push commits since it makes incremental review harder (we squash them at the end anyway).

@ntouran
Copy link
Contributor Author

ntouran commented Oct 12, 2023

Perfect! In the future, if you want to contribute again, don't force push commits since it makes incremental review harder (we squash them at the end anyway).

Ok sorry. The 2nd commit on the followup had the wrong email address, so I only force pushed an update to the email to that one commit, hoping the tooling would show just that, but I see what you're saying, and can understand that.

@AA-Turner AA-Turner merged commit 2d1b943 into sphinx-doc:master Jan 8, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2024
@AA-Turner AA-Turner added this to the 7.3.0 milestone Jul 13, 2024
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.

3 participants