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

Fix/timeout #102

Merged
merged 5 commits into from
Apr 3, 2024
Merged

Fix/timeout #102

merged 5 commits into from
Apr 3, 2024

Conversation

volodymyrZotov
Copy link
Contributor

@volodymyrZotov volodymyrZotov commented Apr 3, 2024

This PR fixes the issue when Client throws an error if OP_CONNECT_CLIENT_REQ_TIMEOUT env var is not set.

In addition get_timeout function is improved to cover the following cases:

  • when export OP_CONNECT_CLIENT_REQ_TIMEOUT=None - it disables all the timeouts (behaves the same as described in to documentation)
  • when export OP_CONNECT_CLIENT_REQ_TIMEOUT='' - it sets default timeout 5s.
  • when export OP_CONNECT_CLIENT_REQ_TIMEOUT=0 -it sets default timeout 5s.

Testing steps

The env var is set

  1. Up 1Password Connect server.
  2. Put this to main.py file
from onepasswordconnectsdk.client import AsyncClient, Client
from onepasswordconnectsdk import new_client

client: Client = new_client(host, token) #put your token an host
vaults = client.get_vaults()
print(vaults)
  1. export OP_CONNECT_CLIENT_REQ_TIMEOUT=30
  2. run python main.py

Should use default timeout

  1. Up 1Password Connect server.
  2. Put this to main.py file
from onepasswordconnectsdk.client import AsyncClient, Client
from onepasswordconnectsdk import new_client

client: Client = new_client(host, token) #put your token an host
vaults = client.get_vaults()
print(vaults)
  1. run python main.py

Resolves #104

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.96%. Comparing base (627f59d) to head (4bcf4d7).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
+ Coverage   76.60%   76.96%   +0.36%     
==========================================
  Files          27       27              
  Lines        1949     1980      +31     
==========================================
+ Hits         1493     1524      +31     
  Misses        456      456              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@volodymyrZotov volodymyrZotov requested a review from edif2008 April 3, 2024 03:47
@raphapassini
Copy link
Contributor

Thanks for fixing this @volodymyrZotov, I've created a PR as well fixing this but you was faster than me.
Sorry for the bug I've introduced :(

@raphapassini
Copy link
Contributor

Fell free to close my PR in favor of yours.

@volodymyrZotov
Copy link
Contributor Author

@raphapassini No worries! I also missed that when tested it. Thanks for the attempt to fix it! That looks like the right approach. But I lean toward merging this one as it also covers edge cases like setting None and non-numeric values.

@volodymyrZotov volodymyrZotov merged commit 0429e1e into main Apr 3, 2024
1 check passed
@volodymyrZotov volodymyrZotov deleted the fix/timeout branch April 3, 2024 16:31
@github-actions github-actions bot mentioned this pull request Apr 3, 2024
2 tasks
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.

'UseClientDefault' object cannot be interpreted as an integer
4 participants