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

Propagate client's adapter to API categories #939

Merged
merged 1 commit into from
Jan 29, 2023

Conversation

WilkenSteiner
Copy link
Contributor

@WilkenSteiner WilkenSteiner commented Jan 16, 2023

Propagate the adapter used by the client to all handlers registered on the client.

Previously, setting the adapter (and thus session/connection pool) on the client after initialization would only replace the adapter owned by the client, leaving the adapters of all API categories untouched.

Notes

  • If the current behaviour was by design, I will extend the docstrings instead. The new behaviour seems more intuitive to me, also more in line with other methods (e.g. the URL setter on the client).
  • Added a regression test.

@WilkenSteiner WilkenSteiner requested a review from a team as a code owner January 16, 2023 07:32
@WilkenSteiner WilkenSteiner changed the title Propagate client's adapter to api categories Propagate client's adapter to API categories Jan 16, 2023
@briantist
Copy link
Contributor

Hi @WilkenSteiner ! Thanks for this submission, I will have to take some time to go over it in a little more detail.

While the original designers are not active here anymore, off the top of my head, I would say that it was probably expected that the adapter would not b re-assigned after Client instantiation, though the fact that the .adapter setter was defined would make that confusing.

So I am inclined to agree that this is more intuitive, but let me think about it a bit and discuss with the other maintainers.

Also huge thanks for adding a regression test!

@briantist briantist self-assigned this Jan 16, 2023
@briantist briantist added bug adapters related to the Adapter system labels Jan 16, 2023
@briantist
Copy link
Contributor

briantist commented Jan 16, 2023

@WilkenSteiner by the way, I've just created a poll and discussion topic about the Adapter system, and I'd appreciate a vote and/or some feedback if you have a few moments:

@WilkenSteiner
Copy link
Contributor Author

@briantist Thank you for considering and putting a more structural spin on it! I left a comment in the poll with my use case.

@briantist
Copy link
Contributor

briantist commented Jan 27, 2023

@WilkenSteiner would you rebase this? The CI should be fixed now I was able to rebase for you

@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Merging #939 (477ebb5) into develop (026d6b5) will increase coverage by 0.30%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #939      +/-   ##
===========================================
+ Coverage    81.34%   81.64%   +0.30%     
===========================================
  Files           65       65              
  Lines         2969     3002      +33     
===========================================
+ Hits          2415     2451      +36     
+ Misses         554      551       -3     
Impacted Files Coverage Δ
hvac/api/system_backend/__init__.py 100.00% <ø> (+3.84%) ⬆️
hvac/v1/__init__.py 87.26% <100.00%> (+3.43%) ⬆️
hvac/api/auth_methods/mfa.py 100.00% <0.00%> (ø)
hvac/api/secrets_engines/kv.py 100.00% <0.00%> (ø)
hvac/api/secrets_engines/identity.py 94.00% <0.00%> (+0.02%) ⬆️
hvac/utils.py 77.27% <0.00%> (+0.20%) ⬆️
hvac/adapters.py 86.73% <0.00%> (+0.27%) ⬆️
hvac/api/vault_api_category.py 95.12% <0.00%> (+0.67%) ⬆️

@briantist briantist merged commit 9fedf49 into hvac:develop Jan 29, 2023
briantist pushed a commit to briantist/hvac that referenced this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapters related to the Adapter system bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants