-
Notifications
You must be signed in to change notification settings - Fork 152
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
Add timeout to requests
calls
#617
base: main
Are you sure you want to change the base?
Conversation
Quality Gate passedIssues Measures |
@johnyrahul @muhammad-ali-e @gaya3-zipstack Looks good overall. |
@@ -52,7 +52,7 @@ def make_get_request( | |||
""" | |||
url = f"{self.base_url}{endpoint}" | |||
params = {"image_name": image_name, "image_tag": image_tag} | |||
response = requests.get(url, params=params) | |||
response = requests.get(url, params=params, timeout=60) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here It might get issue because, some tools need to be run more than 60 seconds. It's better to do with some env variables,
NIT: We also need to ensure we catch and handle Timeout exception. FYI we also have such calls made across other repos as well which might need a review too |
Quality Gate passedIssues Measures |
If there is another repo you have in mind that you'd like for me to scan with pixeebot, feel free to link it here. Thanks, |
What, Why & How
Many developers will be surprised to learn that
requests
library calls do not include timeouts by default. This means that an attempted request could hang indefinitely if no connection is established or if no data is received from the server.The requests documentation suggests that most calls should explicitly include a
timeout
parameter. This codemod adds a default timeout value in order to set an upper bound on connection times and ensure that requests connect or fail in a timely manner. This value also ensures the connection will timeout if the server does not respond with data within a reasonable amount of time.While timeout values will be application dependent, we believe that this codemod adds a reasonable default that serves as an appropriate ceiling for most situations.
Our changes look like the following:
More reading
Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.
🧚🤖 Powered by Pixeebot