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

S3 localstack ize #266

Open
wants to merge 42 commits into
base: master
Choose a base branch
from
Open

S3 localstack ize #266

wants to merge 42 commits into from

Conversation

dmichaels-harvard
Copy link
Contributor

@dmichaels-harvard dmichaels-harvard commented Jun 28, 2023

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.

@coveralls
Copy link

coveralls commented Jun 29, 2023

Pull Request Test Coverage Report for Build 5978776288

  • 34 of 35 (97.14%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 77.809%

Changes Missing Coverage Covered Lines Changed/Added Lines %
dcicutils/boto_monkey_patching.py 33 34 97.06%
Totals Coverage Status
Change from base Build 5943632943: 0.06%
Covered Lines: 8268
Relevant Lines: 10626

💛 - Coveralls

Copy link
Member

@willronchetti willronchetti left a 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

Comment on lines 16 to 17
{"service": "s3", "env": "LOCALSTACK_S3_URL"},
{"service": "sqs", "env": "LOCALSTACK_SQS_URL"}
Copy link
Member

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

Copy link
Contributor Author

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
Comment on lines 3 to 7
# 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")
Copy link
Member

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.

Copy link
Contributor Author

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.

@@ -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'
Copy link
Collaborator

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.

Suggested change
'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',

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.

4 participants