-
Notifications
You must be signed in to change notification settings - Fork 394
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
S3 access_key_id & secret_access_key args #1589
Merged
Merged
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
2d9dc4f
S3 access_key_id & secret_access_key args
farizrahman4u e0bc286
Update content/docs/command-reference/remote/modify.md
jorgeorpinel 270010c
Update content/docs/command-reference/remote/modify.md
farizrahman4u 747f712
Update content/docs/command-reference/remote/modify.md
farizrahman4u 9af97ca
ws fix
farizrahman4u 1b7e279
better explanation
farizrahman4u 0dfe154
ws fix
farizrahman4u 56611c6
Update content/docs/command-reference/remote/modify.md
jorgeorpinel 3d5fc40
Update content/docs/command-reference/remote/modify.md
jorgeorpinel bdc961d
Update content/docs/command-reference/remote/modify.md
jorgeorpinel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
But actually... All of these settings are optional, even
credentialpath
. So why/when doescredentialpath
need to be overridden?From the section's description: "To override some of these settings, you could use the following options".
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.
Are these new ones just an alternative to
credentialpath
?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.
iterative/dvc#4175
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.
Yes, all of these are optional except for the url. You can override default credentialpath (~/.aws/credentials) when you want to use a different creds file.
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.
Thanks, I get it. My questions are geared towards improving the explanation for the users.
So maybe
credentialpath
should state it is the default AWS credentials file if omitted, or something like this? Another option is to say these new options can be used "instead ofcredentialpath
" (and not call them optional). The way they're described now may be confusing.This comment was marked as resolved.
Sorry, something went wrong.
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.
These config options will override not only what is in credentialpath but also what is in the env or boto3-specific config, so the hierarchy is more complex and, ideally, it should be described (a note about us using boto3 and a reference to https://boto3.amazonaws.com/v1/documentation/api/latest/guide/configuration.html might suffice) once for the whole s3-related section. We could create a ticket for that for later, but for now just say that these newly added options are optional.
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.
Sure, feel free to open the ticket. Seems like a note we should have... For now I applied my suggestion and merged this. It does already imply all these settings are optional at the beginning of the section, and per the nature of
dvc remote modify
.