Skip to content

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.

],

'Other/Proprietary License': [
'localstack-ext'
Copy link
Contributor

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.

5 participants