-
Notifications
You must be signed in to change notification settings - Fork 86
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
Handle S3 credential expiration more gracefully #354
Conversation
👈 Launch a binder notebook on this branch for commit 64c0976 I will automatically update this comment whenever this PR is modified 👈 Launch a binder notebook on this branch for commit a33bf3a 👈 Launch a binder notebook on this branch for commit ab70e65 👈 Launch a binder notebook on this branch for commit ce741a8 👈 Launch a binder notebook on this branch for commit ad85cd4 👈 Launch a binder notebook on this branch for commit 6c342a2 |
@MattF-NSIDC @betolink I confirmed the changes here work as expected in practice (i.e. no longer getting the permissions errors reported in the #353) |
It may realistically be a while before I can review this! 😢 |
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.
Great work! I have a couple thoughts about readability, but no concerns about the behavior at the moment.
I don't think the suggestions I made are significant enough to warrant a second review, so if you're ready to merge after going through them: 🚀
related to this, just for reference, Alex Mandel from DevSeed mentioned this piece of code that handles more or less the same for the MAAP project https://github.com/MAAP-Project/gedi-subsetter/blob/58c87ab907bc3b121510eeabdeb2b8e63c7df999/src/gedi_subset/maapx.py#L55-L61 |
Co-authored-by: Matt Fisher <mfisher87@gmail.com>
Nice, thanks for pointing us to that @betolink. Good to know for future reference. |
@mfisher87 I've addressed or responded to all your review comments (thank you for taking the time to review this PR). There are a few minor, subjective points where I think we have differing opinions / styles -- let me know if any of those are blocking comments. |
@jrbourbeau thanks! I don't feel any of those differing opinions / styles should be blockers, marked all as resolved! I think this is ready to go 💯 |
@@ -209,8 +209,15 @@ def get_s3fs_session( | |||
"parameters must be specified. " | |||
) | |||
|
|||
if concept_id is not None: | |||
provider = self._derive_concept_provider(concept_id) |
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.
💯
Closes #353