Skip to content
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: Use key_id and key_secret directly #4224

Merged
merged 14 commits into from
Jul 19, 2020

Conversation

farizrahman4u
Copy link
Contributor

@farizrahman4u farizrahman4u commented Jul 17, 2020

Fixes #4175

@efiop

@efiop
Copy link
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. 🙂

dvc/tree/s3.py Outdated Show resolved Hide resolved
dvc/tree/s3.py Outdated Show resolved Hide resolved
dvc/tree/s3.py Outdated Show resolved Hide resolved
@efiop
Copy link
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
Contributor Author

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

@skshetry
Copy link
Member

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
Contributor Author

@efiop Done.

dvc/config.py Outdated Show resolved Hide resolved
Co-authored-by: Ruslan Kuprieiev <[email protected]>
tests/unit/remote/test_s3.py Outdated Show resolved Hide resolved
dvc/config.py Outdated Show resolved Hide resolved
@efiop
Copy link
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
Contributor Author

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

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
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
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
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 iterative: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