-
Notifications
You must be signed in to change notification settings - Fork 1
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
S3 localstack ize #266
base: master
Are you sure you want to change the base?
S3 localstack ize #266
Conversation
… with S3_URL/SQS_URL env vars.
Pull Request Test Coverage Report for Build 5978776288
💛 - Coveralls |
…LOCALSTACK_SQS_URL rather than S3_URL and SQS_URL.
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.
Code looks good but I think requires build/install instructions for localstack, will approve when that is added
dcicutils/boto_monkey_patching.py
Outdated
{"service": "s3", "env": "LOCALSTACK_S3_URL"}, | ||
{"service": "sqs", "env": "LOCALSTACK_SQS_URL"} |
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.
You might want constant for LOCALSTACK_S3_URL
etc so can be imported and patched from pytest easily
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.
Ah yes, thanks.
test/conftest.py
Outdated
# Disable any AWS endpoint-url environment variables overrides for testing. | ||
if "S3_URL" in os.environ: | ||
os.environ.pop("S3_URL") | ||
if "SQS_URL" in os.environ: | ||
os.environ.pop("SQS_URL") |
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.
Does this not globally eliminate these values? You probably want a session scoped fixture that pops these values back on and off, just in case? Though probably not a big deal in local environment.
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.
Yeah, I was a little unsure of this - was doing this because when I had this env vars set in my env, because I was using this facility for testing locally with localstack, tests were failing (because tests not setup of course with the assumption of using localstack), so I just globally unset these variables for all testing.
… localstack to dev-dependencies in pyproject.toml.
… localstack to dev-dependencies in pyproject.toml.
@@ -853,6 +862,10 @@ class C4InfrastructureLicenseChecker(LicenseChecker): | |||
'typed-function', # LICENSE at https://www.npmjs.com/package/typed-function?activeTab=code | |||
|
|||
], | |||
|
|||
'Other/Proprietary License': [ | |||
'localstack-ext' |
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.
Needs a comment. Please don't add any item to these lists without supporting rationale.
Also, needs a trailing comma so you don't have to change an extra line to add to the list.
'localstack-ext' | |
# This is known to be offered under Apache-2.0 license. | |
# Ref: https://github.com/localstack/localstack/blob/master/LICENSE.txt | |
'localstack-ext', |
….4.1) or 6 and up.
Provides ability to use the "localstack" utility to run/emulate AWS S3 and SQS locally, via the LOCALSTACK_S3_URL and LOCALSTACK_SQS_URL environment variables. Also (as a BTW) updates PyYAML to ^6.0.1 because Mac M1 (with Python 3.9) only likes 5.3.1 (not 5.4.1) or 6 and up.