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

DM 21015: Fix AWS mock credentials. #184

Merged
merged 4 commits into from Aug 21, 2019
Merged

Conversation

DinoBektesevic
Copy link
Collaborator

When trying to run the S3 datastore tests, with boto3 and moto installed, they fail unless some values are present in a ~/.aws/credentials file.

Tests now check whether the file or if the environmental variables are set and if not sets them. The values are unset at the end of the tests.

@DinoBektesevic DinoBektesevic self-assigned this Aug 19, 2019
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

I have two main comments:

  1. It looks to me like this entire test case is mocked out so why all the work to pass through real credentials?
  2. If we are trying to pass through real credentials then there is too much duplicated code here that should be moved to a single location: s3utils seems plausible.

tests/test_butler.py Outdated Show resolved Hide resolved
tests/test_butler.py Outdated Show resolved Hide resolved
tests/test_butler.py Outdated Show resolved Hide resolved
tests/test_butler.py Outdated Show resolved Hide resolved
tests/test_butler.py Outdated Show resolved Hide resolved
tests/test_butler.py Outdated Show resolved Hide resolved
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Looks good. One minor clarification and one tweak needed before merging.

python/lsst/daf/butler/core/s3utils.py Show resolved Hide resolved
tests/test_butler.py Outdated Show resolved Hide resolved
@DinoBektesevic DinoBektesevic merged commit b7fdf8b into master Aug 21, 2019
@timj timj deleted the tickets/DM-21015 branch April 22, 2020 22:03
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.

None yet

2 participants