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

chore(pre-commit): autoupdate hooks #1314

Merged
merged 4 commits into from Mar 27, 2023
Merged

Conversation

pre-commit-ci[bot]
Copy link
Contributor

@pre-commit-ci pre-commit-ci bot commented Mar 13, 2023

updates:
- [github.com/pycqa/bandit: 1.7.4 → 1.7.5](PyCQA/bandit@1.7.4...1.7.5)
@pre-commit-ci pre-commit-ci bot requested a review from a team as a code owner March 13, 2023 21:01
@thekaveman
Copy link
Member

This is interesting!

https://bandit.readthedocs.io/en/1.7.5/plugins/b113_request_without_timeout.html

This plugin test checks for requests calls without a timeout specified.
...
New in version 1.7.5.

Hence the pre-commit failure here.

@machikoyasuda
Copy link
Member

From dev meeting: Let’s add timeouts to our requests so Bandit passes these tests

@angela-tran
Copy link
Member

I was looking into what value we should use for the timeout. According to request docs, we can set values for the "connect" timeout and the "read" timeout.

I think we might want to have these driven by configurable settings rather than hard-coding them. So basically we would introduce new environment variables for these two values, update docs, and use them everywhere that we make an HTTP request via requests.

@thekaveman
Copy link
Member

Let's keep this as simple as possible. I don't think we have any reason or data to suggest we need to worry about timeouts anywhere in the app. Fine to create an env var, but let's have a sane default and not expect that we'll need to override it really ever, unless this becomes a real issue.

@angela-tran angela-tran self-assigned this Mar 17, 2023
@angela-tran
Copy link
Member

The bandit failure should be addressed by the commits I pushed. However, this is blocked due to the same error that is blocking #1325

@angela-tran
Copy link
Member

This is now unblocked and ready for review

Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

I was looking around the web at different ways to set this timeout, hoping for a simpler configuration that would avoid having to repeat the setting each time. Nothing compelling came up. A Session object doesn't feel correct since we're requesting from many different services.

This works! 👍

@angela-tran angela-tran merged commit e890af6 into dev Mar 27, 2023
12 checks passed
@angela-tran angela-tran deleted the pre-commit-ci-update-config branch March 27, 2023 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants