S3 access_key_id & secret_access_key args#1589
Conversation
jorgeorpinel
left a comment
jorgeorpinel
left a comment
There was a problem hiding this comment.
Thanks @farizrahman4u please request my review when treeverse/dvc/pull/4224 is merged.
jorgeorpinel
changed the title
|
Another note: something is wrong with the text formatting here because a Restyled PR was created automatically. I thought it was the white spaces but I removed them and the Restyled PR is still there (it should close automatically). Please try to figure out what the difference is comparing your changes to https://github.com/iterative/dvc.org/pull/1590/files and/or follow our contribution guidelines, found in https://dvc.org/doc/user-guide/contributing/docs. Thanks |
Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
|
@jorgeorpinel Styling seems to be fixed now. |
| - `access_key_id` - (optional) AWS Access Key ID. Overrides credentials from | ||
| `credentialpath`: | ||
|
|
||
| ```dvc | ||
| $ dvc remote modify myremote access_key_id my-access-key-id | ||
| ``` | ||
|
|
||
| - `secret_access_key` - (optional) AWS Secret Access Key. Overrides credentials | ||
| from `credentialpath`: |
There was a problem hiding this comment.
But actually... All of these settings are optional, even credentialpath. So why/when does credentialpath 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.
Are these new ones just an alternative to credentialpath?
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
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 of credentialpath" (and not call them optional). The way they're described now may be confusing.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
We could create a ticket for that for later, but for now just say that these newly added options are optional.
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.
jorgeorpinel
left a comment
jorgeorpinel
left a comment
There was a problem hiding this comment.
One more question left please ☝️
jorgeorpinel
changed the title
jorgeorpinel
left a comment
jorgeorpinel
left a comment
There was a problem hiding this comment.
Applied my suggestion.
|
Merged the Restyled PR (#1603). |
3 participants
treeverse/dvc#4224