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

gdrive: add gdrive_user_credentials_file description #1192

Merged
merged 5 commits into from
Apr 28, 2020

Conversation

shcheklein
Copy link
Member

Corresponding core DVC PR iterative/dvc#3686


You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please chose to allow us to edit your branch when creating the PR.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@shcheklein shcheklein temporarily deployed to dvc-landing-gdrive-mult-qm5n5n April 27, 2020 01:15 Inactive
@shcheklein shcheklein requested a review from jorgeorpinel April 27, 2020 01:15
@shcheklein shcheklein temporarily deployed to dvc-landing-gdrive-mult-qm5n5n April 27, 2020 01:17 Inactive
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Suggestions and questions

content/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
content/docs/command-reference/remote/modify.md Outdated Show resolved Hide resolved
Comment on lines +202 to +203
`.dvc/tmp/gdrive-user-credentials.json` and they will be used automatically next
time you run DVC.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`.dvc/tmp/gdrive-user-credentials.json` and they will be used automatically next
time you run DVC.
`.dvc/tmp/gdrive-user-credentials.json` by default, and they will be loaded
automatically next time you use the same GDrive remote.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is not exactly true, they can be used for multiple remotes if you don't specify an option gdrive_user_credentials_file

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. But "run DVC" doesn't sound exact either. How about

Suggested change
`.dvc/tmp/gdrive-user-credentials.json` and they will be used automatically next
time you run DVC.
`.dvc/tmp/gdrive-user-credentials.json` by default, and they will be loaded
automatically next time you use this or other GDrive remotes.

Maybe even continue this paragraph with the explanation about how to overwrite it, instead of a separate paragraph that repeats the file name.

content/docs/user-guide/setup-google-drive-remote.md Outdated Show resolved Hide resolved
@shcheklein shcheklein requested a review from jorgeorpinel April 28, 2020 02:58
@shcheklein shcheklein temporarily deployed to dvc-landing-gdrive-mult-qm5n5n April 28, 2020 02:58 Inactive
Comment on lines +209 to +210
If you use multiple GDrive remotes, by default they will be sharing the same
`.dvc/tmp/gdrive-user-credentials.json` credentials file. It can be overridden
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we don't need to repeat the file name since it's mentioned in in the previous paragraph (before the warning).

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

I just thing the part around the credentials file name and how to overwrite it could be merged into a single paragraph or something like that to avoid repetition and going back and forth on the same idea.

@shcheklein shcheklein temporarily deployed to dvc-landing-gdrive-mult-qm5n5n April 28, 2020 22:10 Inactive
@shcheklein shcheklein merged commit cba33a3 into master Apr 28, 2020
@jorgeorpinel jorgeorpinel deleted the gdrive-multi-remote branch May 5, 2020 22:39
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.

2 participants