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

Drop use of maap-py for obtaining S3 credentials #71

Merged
merged 4 commits into from
May 9, 2024

Conversation

chuckwondo
Copy link
Collaborator

S3 credentials are now obtained via a role using EC2 instance metadata. In addition, removed HTTP retry logic since all granules are now obtained via S3 only.

Fixes #14

S3 credentials are now obtained via a role using EC2
instance metadata.

Fixes #14
@chuckwondo chuckwondo marked this pull request as draft May 7, 2024 18:47
@wildintellect
Copy link
Contributor

I'm generally in favor of this for actual MAAP usage, but have concerns about removing the functionality to operate in a different way given the possibility of wanting to us this outside of MAAP, e.g. VEDA.
That would favor dropping maap-py but I wonder if we can more generically make AWS credentials modular to allow several options for obtaining (e.g. EarthAccess)

@chuckwondo
Copy link
Collaborator Author

I'm generally in favor of this for actual MAAP usage, but have concerns about removing the functionality to operate in a different way given the possibility of wanting to us this outside of MAAP, e.g. VEDA. That would favor dropping maap-py but I wonder if we can more generically make AWS credentials modular to allow several options for obtaining (e.g. EarthAccess)

Yes, there is a separate issue for refactoring for use outside of MAAP. When I work on that issue, I'll sort out how to make this modular. At the moment, I feel this simplification will better enable introducing modularization later. There will be less cruft to deal with.

@chuckwondo chuckwondo force-pushed the issue14-auto-refresh-s3-creds branch from 8e28e8f to fb650db Compare May 8, 2024 12:18
@chuckwondo chuckwondo marked this pull request as ready for review May 8, 2024 12:20
@chuckwondo
Copy link
Collaborator Author

@wildintellect and @jjfrench, I've taken this PR out of Draft. It is now ready for review. I registered gedi-subset:issue14-auto-refresh-s3-creds and confirmed that it works.

bin/build-dps Outdated Show resolved Hide resolved
Copy link
Collaborator

@jjfrench jjfrench left a comment

Choose a reason for hiding this comment

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

Lookin good, thanks!

@chuckwondo chuckwondo merged commit bd3ae00 into main May 9, 2024
2 checks passed
@chuckwondo chuckwondo deleted the issue14-auto-refresh-s3-creds branch May 9, 2024 19:17
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.

Make S3 credentials caching/refresh logic more robust
3 participants