-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
config: Support ~/.aws/config parsing #5378
Conversation
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.
LGTM!
@@ -0,0 +1,24 @@ | |||
# https://github.com/aws/aws-cli/blob/5aa599949f60b6af554fd5714d7161aa272716f7/awscli/customizations/s3/utils.py |
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.
Since this whole file is awscli-specific, we could move this stuff to dvc/tree/s3.py
or create something like dvc/tree/s3/utils.py
and it put it there. This piece is quite small, so could go with the former, no problem.
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.
I think keeping here is OK (even though this is a pre-emptive assumption) I believe this utility might come in handy in other places since it does a very general job. The awscli-specific part here is the handling of IEC suffixes, though it doesn't differ much and something we can ignore.
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.
Ok, let's keep it then π
β I have followed the Contributing to DVC checklist.
π If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. (config: add docs for s3.configpathΒ dvc.org#2140)
Thank you for the contribution - we'll try to review it as soon as possible. π
resolves #4359