Skip to content

Conversation

@cloutierMat
Copy link
Member

@cloutierMat cloutierMat commented Apr 17, 2025

Motivation

Investigating issues with ApiGateway sending wrong Accept-Encoding headers to it's integration, with @bentsku, we found urllib3.util.SKIP_HEADER which let's us prevent requests from adding the specified header.

Changes

  • Adds SKIP_HEADER instead of identity when the Accept-Encoding header isn't set
  • Adds tests

@cloutierMat cloutierMat marked this pull request as ready for review April 17, 2025 20:59
@cloutierMat cloutierMat requested a review from thrau April 17, 2025 21:00
@bentsku bentsku requested a review from alexrashed May 12, 2025 12:14
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! This will allow us to remove some snapshot skips in upstream dependencies, and is a "more correct" way of handling the Accept-Encoding header. The identity workaround is known, but we can have better parity and identical behaviors with this.
TIL about this header, you already showed it to me in APIGW and it really helps! 🚀

I'll leave the final review to @alexrashed, but this has been proven to work in APIGW NextGen already 😄

Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Also looks good to me! Happy to merge this as it is!
Is this change already being awaited / should we create a release right away?

@alexrashed alexrashed merged commit 7dd568b into main May 12, 2025
4 checks passed
@alexrashed alexrashed deleted the remove-accept-encoding-header branch May 12, 2025 13:41
@cloutierMat
Copy link
Member Author

As far as I know the only follow up upstream will be to cleanup some snapshot tests, unless @bentsku is actively working on those there is no rush from my part.

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.

4 participants