Conversation
github-actions
Bot
added
area/local/start-api
| """ | ||
| try: | ||
| get_request = requests.get(uri, stream=True, verify=os.environ.get("AWS_CA_BUNDLE", True)) | ||
| get_request = requests.get(uri, stream=True, verify=os.environ.get("AWS_CA_BUNDLE", "True")) |
There was a problem hiding this comment.
This is actually wrong.
requests.get (which behind the scenes just calls requests.request) has this verify parameter that can be either a path to a CA bundle to use or a boolean (https://requests.readthedocs.io/en/latest/api/#requests.request)
So it doesn't really accept a "True" string.
If ruff is complaining about this (which kind of make sense because we're returning two different types from one os.environ.get), we can do maybe
, verify=os.environ.get("AWS_CA_BUNDLE") or True)Or just add a comment to ignore the rule for this line.
| path_mock.unlink.assert_called() | ||
| unzip_patch.assert_called_with("layer_zip_path", "output_zip_dir", permission=0o700) | ||
| os_patch.environ.get.assert_called_with("AWS_CA_BUNDLE", True) | ||
| os_patch.environ.get.assert_called_with("AWS_CA_BUNDLE", "True") |
There was a problem hiding this comment.
I guess you'll have to change this test if you change the behavior of os.environ.get in the other file, but this is probably not it.
| LOG = logging.getLogger(__name__) | ||
|
|
||
| CONTAINER_CONNECTION_TIMEOUT = float(os.environ.get("SAM_CLI_CONTAINER_CONNECTION_TIMEOUT", 20)) | ||
| CONTAINER_CONNECTION_TIMEOUT = float(os.environ.get("SAM_CLI_CONTAINER_CONNECTION_TIMEOUT", "20")) |
There was a problem hiding this comment.
(Just as a small rant incoming, but this code is okay how you did it)
Conceptually this is weird, but I imagine this is the easiest way to do it.
Because we're passing a "20" (string) that will be converted to a number with float(..).. But in an ideal world we just want to pass the number right away instead of creating the intermediate string.
What we would want is to do something like
CONTAINER_CONNECTION_TIMEOUT = float(os.environ.get("SAM_CLI_CONTAINER_CONNECTION_TIMEOUT")) if "SAM_CLI_CONTAINER_CONNECTION_TIMEOUT" in os.environ else 20
(check if the envvar exists, and only in that case convert to float, otherwise pass the number directly)
but that's terrible code too, so we can just keep it with what you did here.
|
Address in this #7947 |
3 participants
Which issue(s) does this change fix?
Address PLW1508. This is needed to merge #7947. Currently, some checks are failing on the PR.
Why is this change necessary?
How does it address the issue?
What side effects does this change have?
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.