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

Fix #3550: OCM share fails for json driver #3551

Merged
merged 3 commits into from
Jan 10, 2023
Merged

Conversation

redblom
Copy link
Contributor

@redblom redblom commented Dec 12, 2022

Fixes implementation omission in ocm share json driver in #3526

@redblom
Copy link
Contributor Author

redblom commented Dec 12, 2022

Ps. Tested locally. Both OCM sharing and transfers successful.

Copy link
Contributor

@michielbdejong michielbdejong left a comment

Choose a reason for hiding this comment

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

Thanks! Sorry if this broke for you! We were in a bit of a hurry to merge last week. :)

One remark: I notice you used fmt.Sprintf("%s@%s", userID.OpaqueId, userID.Idp) instead of fmt.Sprintf("%s@%s", userID.OpaqueId, sm.webDAVHost) which may be fine depending on which service is used as the WebDAV server, right?

I guess if you run Reva's ocdav module then userID.Idp works?
Does this work with the ocdav module?

@redblom
Copy link
Contributor Author

redblom commented Dec 13, 2022

One remark: I notice you used fmt.Sprintf("%s@%s", userID.OpaqueId, userID.Idp) instead of fmt.Sprintf("%s@%s", userID.OpaqueId, sm.webDAVHost) which may be fine depending on which service is used as the WebDAV server, right?

I guess if you run Reva's ocdav module then userID.Idp works? Does this work with the ocdav module?

Well, originally this was the 'meshProvider' (user.idp), this fix still works for this driver with ocdav. The webDAVHost is not available in the json driver.
Is the ratio behind (sending) this (new) webDAVHost config by the nextcloud driver discussed somewhere? Can't seem to find that.

@michielbdejong
Copy link
Contributor

Is the ratio behind (sending) this (new) webDAVHost config by the nextcloud driver discussed somewhere? Can't seem to find that.

Good point, it wasn't so far but I added
this page
to the Reva configuration docs just now.

I created https://reva.link/docs/config/grpc/services/ocmshareprovider/ and so far listed only the Nextcloud driver.

That explains how to configure your EFSS as the webdav host. About the discussion about whether the EFSS or Reva should act as webdav host, I think from the start the plan was to make Reva the webdav host, but I felt that this would just become too slow. I must I haven't tried using Reva with ocdav in front of the EFSS, I guess in theory it should work!

@glpatcern
Copy link
Member

That explains how to configure your EFSS as the webdav host. About the discussion about whether the EFSS or Reva should act as webdav host, I think from the start the plan was to make Reva the webdav host, but I felt that this would just become too slow. I must I haven't tried using Reva with ocdav in front of the EFSS, I guess in theory it should work!

I agree it does make sense to expose the "real" EFSS as the webdav host. We'd expose Reva when Reva is the EFSS engine (i.e. future OCIS systems or current CERNBox).

@gmgigi96 gmgigi96 merged commit a23c0b6 into cs3org:master Jan 10, 2023
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.

5 participants