Skip to content

S3: Use key_id and key_secret directly#4224

Merged
efiop merged 14 commits into
treeverse:masterfrom
farizrahman4u:fr_s3_key_api
Jul 19, 2020
Merged

S3: Use key_id and key_secret directly#4224
efiop merged 14 commits into
treeverse:masterfrom
farizrahman4u:fr_s3_key_api

Conversation

@farizrahman4u
Copy link
Copy Markdown
Contributor

@farizrahman4u farizrahman4u commented Jul 17, 2020

Fixes #4175

@efiop

@efiop
Copy link
Copy Markdown
Contributor

efiop commented Jul 17, 2020

Hi @farizrahman4u !

Thank you for the PR! Please take a look at our contribution guide (it was in the PR hints). Right now our patch checks are failing, because you didn't have pre-commit hooks installed. 🙂

Comment thread dvc/tree/s3.py Outdated
Comment thread dvc/tree/s3.py
Comment thread dvc/tree/s3.py Outdated
@efiop
Copy link
Copy Markdown
Contributor

efiop commented Jul 17, 2020

Also, please feel free to mark your PR as WIP/draft if it is not ready for review yet 🙂 Thanks!

@farizrahman4u farizrahman4u marked this pull request as draft July 17, 2020 05:20
@farizrahman4u
Copy link
Copy Markdown
Contributor Author

@efiop CI seems to be cloning the main repo instead of the fork?

@skshetry
Copy link
Copy Markdown
Collaborator

Looks like it is trying to run restyled's PR and is failing as the branch is no longer there.
@farizrahman4u, please rebase this PR. Probably, that will fix the issue.

@farizrahman4u
Copy link
Copy Markdown
Contributor Author

@efiop Done.

Comment thread dvc/config.py Outdated
Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com>
Comment thread tests/unit/remote/test_s3.py Outdated
Comment thread dvc/config.py Outdated
@efiop
Copy link
Copy Markdown
Contributor

efiop commented Jul 18, 2020

Btw, @farizrahman4u , in the PR header that you've removed, there was a bulletpoint about docs. In particular, we need to add the new config options to https://dvc.org/doc/command-reference/remote/modify (there is an edit button). Could you please submit a docs PR after we are done with config option naming here? 🙂

farizrahman4u added a commit to farizrahman4u/dvc.org that referenced this pull request Jul 18, 2020
@farizrahman4u
Copy link
Copy Markdown
Contributor Author

@efiop done renames and docs pr : treeverse/dvc.org#1589

Comment thread dvc/tree/s3.py
Comment on lines +57 to +58
self.access_key_id = config.get("access_key_id")
self.secret_access_key = config.get("secret_access_key")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we assign it instead of using self.config directly later?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Following the pattern in other remotes (OSS, GDrive etc).

@efiop efiop marked this pull request as ready for review July 19, 2020 10:12
Copy link
Copy Markdown
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Thank you @farizrahman4u !

@efiop efiop merged commit 5825e95 into treeverse:master Jul 19, 2020
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.

s3: provide a way to use key id and key secret directly

5 participants