-
Notifications
You must be signed in to change notification settings - Fork 397
gdrive: Service account support; Default credentials; Trash only conf… #1096
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
Conversation
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.
Some doubts and copy edits. Thanks
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.
the whole doc (user guide one) is a mix of 4 sections which follow each other in some random order w/o explanations and clear connection, some of the should be adjusted (to mention that secret id is optional). Probably optional configure Google Project section should go last, introduction should be extremely clear that you usually need to specify the path (the same in the remote add/modify
)
Thanks @maxhora I think we'll take this over now 👍 |
I think I've mostly fixed this now. Some changes pending, about to push up. Will re-request your review soon, Ivan.
Since we're highly recommending it, I didn't move it last. But I did make it extra clear how easy it is to start using GDrive remotes now both in the guide and in the remote add cmd ref. |
fastest route to start using GDrive remotes
cmd refs as well as in user guide per #1096 (review)
fixed!
that's what S3 cache is for! @fabiosantoscode and @pavelgrinchenko are still looking into these issues with cache. It should be way faster - similar to blog 2-3 minutes. |
can only be referenced by ID). | ||
|
||
```dvc | ||
$ dvc remote add gdfolder gdrive://0AIac4JZqHhKmUk9PDA |
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.
should we add -d
here and in other places?
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.
I intentionally only added it in the Quick start section where it's follower by dvc push
. I don't think that it adds value in these other examples.
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.
It's not obvious to be honest, if we use -d
in all other places, let's use it everywhere?
Also, still don't see any value in these special names (one place is left).
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.
Names updated now. I accidentally missed those, thanks.
I would rather remove -d from everywhere? The topic of default remotes seems totally separate fom gdrive to me. But then if I remove it from the Quick start example, I then need to add it to dvc push -r myremote
... Will do that for now... ⏳
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.
let's def not do dvc push -r
, please ... better to include -d
everywhere ... or even keep as is for now
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.
I changed it to dvc remote add --default just in Quick start for now. That way it's more obvious and no need to explain -d or add it everywhere in this doc?
I noticed the dvc push cmd ref examples need an update and I already started another regular updates PR so I can address it there 🙂
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.
(See #1110)
per #1096 (review) and others
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.
just a minor bug with remote add
left I believe ... check may be I missed some other conversations.
really good stuff!
per #1096 (review) and other comments
Merging, but lmk your final decision about -d and I'll open a quick separate PR. (See #1096 (comment)). Thanks! |
Fixes #1095