tree,remote: add support for WebDAV by iksnagreb · Pull Request #4256 · treeverse/dvc

iksnagreb · 2020-07-22T00:21:52Z

❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

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

Should fix #1153
Took WebDAVURLInfo from #3647
Relates to treeverse/dvc.org#1617

TODO

Tests (at least some unit tests to check whether a valid webdavclient3 can be constructed from config should be doable)
Provide documentation (after discussing the added configuration options for WebDAV)

shcheklein · 2020-07-22T02:05:10Z

should we do >=?

Yes, should be safe, the webdavclient3 API seems to be rather fixed. Just used what I got out of pip freeze...

skshetry · 2020-07-22T12:23:32Z

It'd be great if you could implement walk_files, no pressure though.

skshetry · 2020-07-22T12:29:07Z

You probably don't need root here, as that url from `REMOTE_COMMON should work, no?

Yes, probably it is not necessary any more, I used it to specify the location of the WebDAV API endpoint (e.g. /public.php/webdav). But I still have a bit of confusion there...

Since I now terminate the makedirs at the path part of the initial path_info (which comes from the url of REMOTE_COMMON), the root option might be removed to not confuse others as well.

I somehow still wonder if there could be a valid use case for the option... At least there must be a reason for webdavclient3 to offer this option?

Cannot we split that into hostname and root from the url:
Eg: http://owncloud.com/remote.php/webdav -> http://owncloud.com and /remote.php/webdav?

skshetry · 2020-07-22T14:38:58Z

port is not passed, try something like following, perhaps?

http_info = HTTPURLInfo(self.path_info.url) hostname = http_info.replace(path="").url

Yes, you are right, I somehow forgot that there is more in the path_info than scheme and host... Should be fixed now.

skshetry

Thanks @iksnagreb for the PR. Just tried locally, works for the most part (I hit an issue with port though, see above). Also, I am quite not sure with webdav:// urls though (and, also, root can just be deduced from url as I mentioned above). 🙂

Relates to iterative#1153, treeverse/dvc#4256

efiop · 2020-07-27T00:14:16Z

These are only used in webdav.py, so let's move them there. Also i'm not sure we need a special WebDAVConfigError exception. We could just raise ConfigError or at least inherit from it.

efiop · 2020-07-27T00:16:27Z

Some of these comments are not useful, they are declaring obvious things.

efiop · 2020-07-27T00:19:59Z

I don't think this method is needed.

Yes, it is probably not needed. I have implemented it, because it comes almost for free as it directly matches the webdavclient3 method (the same goes for move/remove). Shall I remove it?

Yeah, let's remove it to not leave dead code around. If we ever need it - it will be trivial to introduce.

efiop · 2020-07-27T00:23:58Z

Shame this library doesn't support progress bars for download/upload, the ui will not be very helpful :(

We need to at least put a dummy progress bar for this.

efiop · 2020-07-27T00:26:08Z

Is etag or something like that part of the webdav standard?

As WebDAV is an extension of HTTP, it should be part of the standard.

Webdav support is based on https://pypi.org/project/webdavclient3/ and supports basic download/upload operation, directory creation as well as existence, file hash and isdir query. Copy, move and remove are also implemented, though probably not used yet. WebdavURLInfo is taken from https://github.com/shizacat/dvc/tree/remote-webdav Fixes iterative#1153

Webdav token auth, certificate and key path and connection timeout are configurable. Webdav username might be specified or extracted from URL. Refs iterative#1153

Refs iterative#1153

This enables the WebDAV api location (e.g. '/public.php/webdav') to be part of the remote 'url' configuration instead of beeing specified separately via the 'root' option. The 'root' option may then be used to specify real directories at the WebDAV storage, although using it to set the api location is still possible. Refs iterative#1153

Context: treeverse#4256 (comment) Refs iterative#1153

The WebDAV 'root' option was rather confusing and should be handled by the initial 'path_info' from the config 'url' option. Context: treeverse#4256 (comment) While stripping the path/root from the hostname the port got lost, which is fixed now by simply using the URLInfo 'replace' method as suggested. Context: treeverse#4256 (comment) The WebDAV client connection is tested by probing the existence of the root (self.path_info.path). Refs iterative#1153

Context: treeverse#4256 (comment) Refs: iterative#1153

Context: treeverse#4256 (comment) Refs iterative#1153

Refs iterative#1153

Context: treeverse#4256 (comment) Refs iterative#1153

Context: treeverse#4256 (comment)

Refs iterative/treeverse#1153

Context: treeverse#4256 (comment) Refs iterative#1153

efiop

This is great! Thank you!

I know that we've discussed tests in discord, but as @pmrowla and @skshetry noted, maybe we could use a docker-based fixture for testing this. E.g. see https://github.com/iterative/dvc/blob/master/tests/remotes/azure.py , where we use azurite docker image declared in https://github.com/iterative/dvc/blob/master/tests/docker-compose.yml . Maybe we could use something like https://hub.docker.com/r/bytemark/webdav to test webdav the same way?

But regardless of the docker-based test, you've confirmed that this implementation works for you, so let's merge for now 🙂

skshetry · 2020-07-30T05:28:13Z

We can also use wsgidav which has a good documentation on how to use it as a library. It requires a wsgi server (eg: gunicorn, etc), but probably we can get away with just wsgiref from the Python's standard library.

shcheklein reviewed Jul 22, 2020

View reviewed changes

skshetry reviewed Jul 22, 2020

View reviewed changes

Comment thread dvc/tree/webdav.py Outdated

Copy link
Copy Markdown

Collaborator

skshetry Jul 22, 2020

Choose a reason for hiding this comment

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

It'd be great if you could implement walk_files, no pressure though.

skshetry reviewed Jul 22, 2020

View reviewed changes

skshetry approved these changes Jul 22, 2020

View reviewed changes

iksnagreb pushed a commit to iksnagreb/dvc.org that referenced this pull request Jul 22, 2020

cmd: add parameters for configuration of WebDAV remotes

a00dd30

Relates to iterative#1153, treeverse/dvc#4256

iksnagreb mentioned this pull request Jul 22, 2020

cmd: add parameters for configuration of WebDAV remotes treeverse/dvc.org#1617

Merged

weekly-digest Bot mentioned this pull request Jul 26, 2020

Weekly Digest (19 July, 2020 - 26 July, 2020) #4285

Closed

efiop reviewed Jul 27, 2020

View reviewed changes

Comment thread dvc/tree/webdav.py Outdated

Copy link
Copy Markdown

Contributor

efiop Jul 27, 2020

Choose a reason for hiding this comment

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

Some of these comments are not useful, they are declaring obvious things.

efiop reviewed Jul 27, 2020

View reviewed changes

Christoph Berganski and others added 5 commits July 27, 2020 15:18

tree,remote: add further webdavclient3 options

66a77e8

Webdav token auth, certificate and key path and connection timeout are configurable. Webdav username might be specified or extracted from URL. Refs iterative#1153

tree,remote: validate webdav client configuration

5c65291

Refs iterative#1153

tree,remote: WebDAV is written with capitalized 'DAV'...

73b5fbe

Refs iterative#1153

iksnagreb added 8 commits July 27, 2020 15:18

tree,remote: use >=3.14.5 for WebDAV dependency webdavclient3

ee1813e

Context: treeverse#4256 (comment) Refs iterative#1153

tree,remote: implement walk_files for WebDAV

4ec6001

Context: treeverse#4256 (comment) Refs: iterative#1153

tree,remote: let WebDAV client list query file info in walk_files

e6c91fb

Context: treeverse#4256 (comment) Refs iterative#1153

tree,remote: add some unit tests for WebDAVTree

f4c368a

Refs iterative#1153

tree,remote: use ConfigError for WebDAVTree and move Error to webdav.py

678b1e7

Context: treeverse#4256 (comment) Refs iterative#1153

tree,remote: remove/change some (unnecessary) comments

5df4829

Context: treeverse#4256 (comment)

tree,remote: uploading to WebDAV only create directories if not exist

68d6ab1

iksnagreb force-pushed the webdav branch from 532259d to 68d6ab1 Compare July 27, 2020 13:20

iksnagreb added 3 commits July 27, 2020 15:26

tree,remote: add BaseTree parameter use_dvcignore to WebDAVTree exists

4f9b34e

Refs iterative/treeverse#1153

tree,remote: remove WebDAVTree copy method as proposed

d2c9d59

Context: treeverse#4256 (comment) Refs iterative#1153

tree,remote: add progress bar to WebDAV _download and _upload method

5203770

Context: treeverse#4256 (comment) Refs iterative#1153

iksnagreb marked this pull request as ready for review July 27, 2020 17:59

efiop approved these changes Jul 29, 2020

View reviewed changes

efiop merged commit 2f48bb4 into treeverse:master Jul 29, 2020

weekly-digest Bot mentioned this pull request Aug 2, 2020

Weekly Digest (26 July, 2020 - 2 August, 2020) #4315

Closed

skshetry mentioned this pull request Aug 11, 2020

remote: test webdav #4374

Closed

efiop mentioned this pull request Sep 15, 2020

remote: support Dropbox #1277

Closed

Conversation

iksnagreb commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iksnagreb Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skshetry left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop left a comment

Choose a reason for hiding this comment

Uh oh!

skshetry commented Jul 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iksnagreb commented Jul 22, 2020 •

edited

Loading

iksnagreb Jul 22, 2020 •

edited

Loading

skshetry left a comment •

edited

Loading