cmd: add parameters for configuration of WebDAV remotes #1617
Conversation
|
Took a quick look. Looking good so far! Please let us know when this is ready for review. |
|
@jorgeorpinel what is the status of this? |
| - `cert_path` - path to certificate used for WebDAV server authentication. | ||
|
|
||
| ```dvc | ||
| $ dvc remote modify myremote cert_path /path/to/cert | ||
| ``` | ||
|
|
||
| - `key_path` - path to private key to use to access a remote. | ||
|
|
||
| ```dvc | ||
| $ dvc remote modify myremote key_path /path/to/key | ||
| ``` |
There was a problem hiding this comment.
Are these alternatives to token and to user/password auth?
There was a problem hiding this comment.
No, according to the WebDAV client documentation (and that is also what I can infer from the implementation), these are additional parameters. But they are optional parameters, probably we should give this information.
There was a problem hiding this comment.
No need to say they're optional. Most parameters except what's shown in the remote add reference are assumed to be optional. But ideally we should briefly explain what these are for, or link to some WebDAV documentation.
There was a problem hiding this comment.
I have added some explanation, is it enough? I don't know what I could link to, these are not really WebDAV specific options - they are just passed to the underlying requests API.
WebDAV seems to be the only remote having such options, even though HTTP is also based on requests it does not expose these option?
There was a problem hiding this comment.
Yep, that was more than enough. Thanks @iksnagreb!
There was a problem hiding this comment.
WebDAV seems to be the only remote having such options, even though HTTP is also based on requests it does not expose these option?
Good question. TBH Idk but maybe @efiop, who I always pick on for these questions 😬, does.
| `user/password` authentication. | ||
|
|
||
| ```dvc | ||
| $ dvc remote modify --local myremote token mytoken |
There was a problem hiding this comment.
Does it need quotes like other string params? I.e.:
| $ dvc remote modify --local myremote token mytoken | |
| $ dvc remote modify --local myremote token "<mytoken>" |
There was a problem hiding this comment.
I don't know, thought it to be more like a variant of user, which has no quotes? But adding quotes should probably be fine. Are there guidelines what parameters are considered as strings/numerics/... and what formatting to apply to each?
There was a problem hiding this comment.
I think it's more of a command line parser thing. All strings can be wrapped with " but if they have special chars or spaces then you definitely need to wrap them. And in fact ' may be better... Idk. I'll just commit this!
There was a problem hiding this comment.
Actually, have you tried this? I assume you tested all the WebDAV params so you can probably tell or double check whether quotes are needed.
There was a problem hiding this comment.
Or if you can rec a quick WebDAV server software to install and run locally for testing, please share 🙂
There was a problem hiding this comment.
Ok, I think you are right, it is better to wrap this in '. There seems to be no standard defining the content and structure of tokens for WebDAV (at least it should be some kind of string). What I can tell for the service I am using (it is basically an onwcloud): it supports OAuth 2.0, probably bearer tokens, which might already contain some basic punctuation characters.
There was a problem hiding this comment.
Thanks for the tip. Resolving Leaving unresolved for future ref.
There was a problem hiding this comment.
Or if you can rec a quick WebDAV server software to install and run locally for testing, please share slightly_smiling_face
Check this link: https://doc.owncloud.com/server/admin_manual/installation/docker/#docker-compose-yaml-file
Copy-paste that into a docker-compose.yml file and then docker-compose up.
There was a problem hiding this comment.
Perfect, thanks @skshetry! Will try this next time its needed.
There was a problem hiding this comment.
Looks good in general but i do have a bunch of small questions now that this is ready for review ☝️ (sorry for the delay, @iksnagreb).
* cmd: remove unnecessary commas in get and import * cmd: fix typo in add * cmd: remote copy edits per #1617 (comment) * guide: .dvcignore copy edit * cmd: init copy edits
|
Merging the Restyled PR, since the core PR is merged and this is approved 🙂 |
* cmd: remove unnecessary commas in get and import * cmd: fix typo in add * cmd: remote copy edits per #1617 (comment) * guide: .dvcignore copy edit * cmd: init copy edits * clarify about dirs in import -o * cmd: review get -o desc * dvcignore: updates to guide and check-ignore ref. per #1629 (review) et al. * cmd: update check-ignore -n per iterative/dvc#4323 (comment) * cmd: fix get.import -o descriptions per #1673 (review) and #1673 (review) * cmd: copy edits to remote add/modify
| ```dvc | ||
| $ dvc remote add -d myremote webdavs://example.com/public.php/webdav | ||
| ``` |
There was a problem hiding this comment.
We just realized that PHP is kind of a weird example to have (see #1695 (review)).
We'll be reviewing these sample URLs in #1706, in case you're interested @iksnagreb 🙂
This adds the documentation of configuration parameters for my proposed addition of WebDAV remotes.
Relates to iterative/dvc#1153, iterative/dvc#4256
❗ 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 choose 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. 🙏