8000 adapters: if `session` is user-supplied, do not overwrite session options with `Client`/`Adapter` options by Tylerlhess · Pull Request #1021 · hvac/hvac · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

adapters: if session is user-supplied, do not overwrite session options with Client/Adapter options #1021

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Sep 16, 2023
Merged

adapters: if session is user-supplied, do not overwrite session options with Client/Adapter options #1021

merged 5 commits into from
Sep 16, 2023

Conversation

Tylerlhess
Copy link
Contributor
@Tylerlhess Tylerlhess commented Jun 30, 2023 •
edited by briantist
Loading

Fixes: #991

Breaking change

For more details see also:

Before this change:

  • if you do not use a custom session, a session is created for you with values for the verify, cert, and proxies settings that are set in the adapter, either explicitly or by default
  • if you do use a custom session:
    • the values in the session will always be overwritten with the adapter values, whether you specified them or not

After this change:

  • if you do not use a custom session, a session is created for you with values for the verify, cert, and proxies settings that are set in the adapter, either explicitly or by default
  • if you do use a custom session:
    • if any value is set for the verify, cert, or proxies properties in your custom session, then that value will be kept and the adapter's value will be ignored, even if you set it on the adapter explicitly
    • if any of those properties are not set then they will be set based on the adapter's value for those

The only case where this should cause breakage of existing code is where all of the following are true:

  • you use a custom session
  • you set any of the affected properties on the custom session
  • the value(s) you set on those properties are actually invalid for your use case, and so your code relied on the adapter defaults overriding your values

@Tylerlhess Tylerlhess requested a review from a team as a code owner June 30, 2023 22:20
@codecov
Copy link
codecov bot commented Jun 30, 2023 •
edited
Loading

Codecov Report

Merging #1021 (3e04a1b) into main (6837bf0) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1021      +/-   ##
==========================================
+ Coverage   85.02%   85.08%   +0.06%     
==========================================
  Files          65       65              
  Lines        3139     3145       +6     
==========================================
+ Hits         2669     2676       +7     
+ Misses        470      469       -1
Files Changed Coverage Δ
hvac/adapters.py 90.35% <100.00%> (+0.53%) ⬆️

... and 1 file with indirect coverage changes

@briantist
Copy link
Contributor

Hey @Tylerlhess thanks for this. I haven't looked at it fully yet, but I wanted to mention I opened #1023 to try to deal with our out-of-date Vault versions in the integration tests.

My hope is we can get all the issues ironed out in there so that you don't have to add these unrelated changes into your PR. I've already borrowed your azure fix (with attribution) in b01e796 though since we're also not pinning to X.Y.0 in that PR anymore, it seems like some changes may have made it into patch versions of Vault, so we may need to be more nuanced in the version compares now.

I really appreciate you bringing these issues to our attention!

@briantist
Copy link
Contributor
briantist commented Jul 3, 2023 •
edited
Loading

With #1023 & #1024 merged, those should fix the Azure failures, SSH failures you saw locally, and the coverage issues, so you can rebase against or merge from the latest main branch HEAD, and remove your azure changes.

I was going to do this for you and push up the branch but for some reason it looks a little weird to me, when I try to rebase your branch all of the files are "conflicts" even the ones you didn't touch. Trying to merge main into your branch refuses due to "unrelated histories" which is probably the same reason for rebase weirdness.

I think I can resolve that anyway and force push, so if you want me to go ahead with that please let me know, otherwise I'll leave it you.

Issue resolved, was on my end.

The other changes in the PR look good to me, the only other thing I'd like to do is add warnings for users who are affected, telling them that in a future major version we'll stop applying any overrides from the affected parameters when a session is supplied.

I can also add that stuff in alongside the work you've done already.

@briantist briantist changed the title fix for #991 adapters: if session is user-supplied, do not overwrite session options with Client/Adapter options Jul 3, 2023
@briantist briantist self-assigned this Jul 3, 2023
@briantist briantist added bug adapters related to the Adapter system client related to the hvac Client labels Jul 3, 2023
@briantist briantist added this to the 1.2.0 milestone Jul 3, 2023
@Tylerlhess
Copy link
Contributor Author

Looks like those changes have resolved the issues that would be happening in 1.12 and 1.13.

@briantist briantist modified the milestones: 1.2.0, 2.0.0 Jul 3, 2023
This was referenced Aug 27, 2023
Merged
Open
@briantist briantist merged commit eaa862e into hvac:main Sep 16, 2023
@briantist briantist mentioned this pull request Oct 14, 2023
Closed
@briantist briantist mentioned this pull request Dec 30, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briantist briantist briantist approved these changes

Assignees

@briantist briantist

Labels
adapters related to the Adapter system breaking-change bug client related to the hvac Client
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.

Custom certificate verification fails with requests>=2.28.0
2 participants
@Tylerlhess @briantist
0