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

Release 2.2.1 #3345

Merged
merged 1 commit into from
Feb 18, 2024
Merged

Release 2.2.1 #3345

merged 1 commit into from
Feb 18, 2024

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented Feb 16, 2024

  • See if all tests, including downstream, pass
  • Get the release pull request approved by a CODEOWNER
  • Squash merge the release pull request with message "Release <VERSION>"
  • Tag with X.Y.Z, push tag on urllib3/urllib3 (not on your fork, update <REMOTE> accordingly)
    • Notice that the <VERSION> shouldn't have a v prefix (Use 1.26.6 instead of v.1.26.6)
    • # Ensure the release commit is the latest in the main branch.
      git checkout main
      git pull origin main
      git tag -s -a '<VERSION>' -m 'Release: <VERSION>'
      git push <REMOTE> --tags
      
  • Execute the publish GitHub workflow. This requires a review from a maintainer.
  • Ensure that all expected artifacts are added to the new GitHub release. Should
    be one .whl, one .tar.gz, and one multiple.intoto.jsonl. Update the GitHub
    release to have the content of the release's changelog.
  • Announce on:
    • Twitter
    • Mastodon
    • Discord
    • OpenCollective
  • Update Tidelift metadata
  • If this was a 1.26.x release, add changelog to the main branch

Copy link
Member

@illia-v illia-v left a comment

Choose a reason for hiding this comment

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

Thanks @pquentin!

@pquentin
Copy link
Member Author

@illia-v mentioned that #3203 made a similar change to the one reverted in #3344, so I need to inspect it before proceeding with the release. At the very least we need to be sure that botocore will work with 2.2.1.

@nateprewitt
Copy link
Member

Confirmed current tip of main works in botocore as expected. Looking at #3203 it's a similar change but appears to only affect the request API and test infrastructure. We aren't using either so I think it's safe to say we're unaffected. The one to request may cause similar issues for other users though.

@pquentin
Copy link
Member Author

pquentin commented Feb 17, 2024

Having looked more closely at #3203, I see two ways to avoid using HTTPHeaderDict. I can ignore the type errors as done in #3344, like this:

diff --git a/src/urllib3/_request_methods.py b/src/urllib3/_request_methods.py
index 632042f0..49495c3e 100644
--- a/src/urllib3/_request_methods.py
+++ b/src/urllib3/_request_methods.py
@@ -122,8 +122,8 @@ class RequestMethods:
                 headers = self.headers
 
             if not ("content-type" in map(str.lower, headers.keys())):
-                headers = HTTPHeaderDict(headers)
-                headers["Content-Type"] = "application/json"
+                headers = headers.copy()  # type: ignore[attr-defined]
+                headers["Content-Type"] = "application/json"  # type: ignore[index]
 
             body = _json.dumps(json, separators=(",", ":"), ensure_ascii=False).encode(
                 "utf-8"

But I would like less ignores, not more. The other option, suggested in python/mypy#5970, is to accept that headers can indeed be immutable given the typing.Mapping type, which means they have to be converted to dicts before being modified:

diff --git a/src/urllib3/_request_methods.py b/src/urllib3/_request_methods.py
index 632042f0..f08b69e8 100644
--- a/src/urllib3/_request_methods.py
+++ b/src/urllib3/_request_methods.py
@@ -122,6 +122,13 @@ class RequestMethods:
                 headers = self.headers
 
             if not ("content-type" in map(str.lower, headers.keys())):
+                # While headers is most likely dict or HTTPHeaderDict, it can
+                # also be an arbitrary immutable mapping that will stay
+                # immutable even after calling copy().
+                if isinstance(headers, HTTPHeaderDict):
+                    headers = headers.copy()
+                else:
+                    headers = dict(headers)
                 headers = HTTPHeaderDict(headers)
                 headers["Content-Type"] = "application/json"

But this is more complicated code for a case that we don't care about (immutable headers object that is not dict or HTTPHeaderDict).

In summary, given that:

  • the above potential "fixes: are undesirable,
  • botocore works with Move test_skip_header to test/test_connection.py #3202 and the proposed release,
  • HTTPHeaderDict not supporting byte values does not affect this part of the code,
  • and that unlike urllib3/connectionpool.py, code in urllib3/_request_methods.py does not have proxy headers of a different type

I'd like to release 2.2.1 as is.

@pquentin pquentin merged commit 54d6edf into urllib3:main Feb 18, 2024
32 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants