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

Sharemanager api changes #2121

Merged
merged 2 commits into from
Oct 7, 2021
Merged

Conversation

butonic
Copy link
Contributor

@butonic butonic commented Sep 30, 2021

This PR introduces the filed mask to the UpdateReceivedShare APIs. This is a breaking change but we tried to change all user and ocm shareprovider managers to use tha field mask.

Unfortunately, the nextcloud driver internally seems to pass on the internal struct. @michielbdejong could you check we correctly implemented a backwards compatible change?

This PR is based on #2023 but reverts everything but the api change.

cc @aduffeck @labkode

@update-docs
Copy link

update-docs bot commented Sep 30, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@butonic butonic force-pushed the sharemanager-api-change branch 7 times, most recently from 536e416 to 84425be Compare October 1, 2021 09:47
@butonic butonic changed the title [WIP] Sharemanager api changes Sharemanager api changes Oct 1, 2021
@butonic butonic marked this pull request as ready for review October 1, 2021 10:01
@butonic butonic requested a review from labkode as a code owner October 1, 2021 10:01
@michielbdejong
Copy link
Contributor

@butonic thanks for the heads-up!
Yeah, definitely please don't delete that test. :)
Here is a way to make it pass: butonic#193
This also makes the function simpler by keeping the paramsObj closer to the function arguments, and just JSON-encoding whatever comes in.

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.

Please have a look at butonic#193

butonic added a commit to butonic/reva that referenced this pull request Oct 1, 2021
@butonic
Copy link
Contributor Author

butonic commented Oct 1, 2021

@michielbdejong Great! Thx!

michielbdejong
michielbdejong previously approved these changes Oct 1, 2021
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.

Changes in pkg/share/manager/nextcloud LGTM now!

@butonic
Copy link
Contributor Author

butonic commented Oct 1, 2021

Note that this PR still needs cs3org/cs3apis#144 ... which has a failing CI ... urgh ... I'll take care of that...

The next step for the shares storahge provider is to introduce field
masks so the shares can update state and mount point. This PR is based
on the full changeset, but removes everything but the API change.

Add a sharesstorageprovider

The provider exposes all received shares. It can be mounted to /home/Shares
to make a lot of special cases for shares handling in the gateway and
storage drivers superfluous.

Add missing mock file

Implement methods for setting/unsetting arbitrary metadata

Fix tests

Adapt to changes from rebase

Fix creating references with embedded mounts

Fix corner cases when stating shares

Allow moves between shares on the same storage

Make the storage rules known to the gateway as well

Do not choke on non-existent shares

Reject a share when it is being deleted

Fix rebase artifacts

WIP: Refactor statting shares. Merge shares permissions.

update after rebase, fix tests

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

list all shares

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

work on api change

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

Fix build

Persist mountpoints in the share managers followin the new cs3 api

Add support for renaming shares in the SharesStorageprovider

Adapt commands for updating received shares

Regenerate the share manager mock

Fix linter warning

Do not raise an internal error when trying to access non-existent shares

Make hound happy

Fix wrong column name in query

Fix typo

Do not confuse user and group names

Add test for listing received group shares

Do not list parent group shares if there is a child share for it already

Make hound happy

Hide the fact that accepted groups shares can be child shares in the db

list shares using the shares manager + hide group shares when the same resource has a user and group share

refactor all the ocs error writing from the new code

Only collide with mountpoints of shares pointing do different resources

Also return shares being shared with one of the user's groups

Add sharesstorageprovider service file for local acceptance tests

Adapt nextcloud share manager to new method signature

Also remove the test for UpdateReceivedShare which can not be tested
anymore with the new signature. The ReceivedShare never held the display
name that's being tested so the test only passed on the data from the
update field, but since the method only takes the actual received share
now this is no longer possible.

WIP: use go-cs3apis fork until it has been merged

Add placeholder changelog to make CI run

Tweak documentation on how to run the acceptance tests

Add missing storage registry rule for the sharesstorageprovider

Fix revad config for local acceptance tests

reduce changes to share api change

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

JSON encode the FieldMask parameter as-is

paramsObj.Ref -> paramsObj.ReceivedShare

Restore deleted test

update code comment

remove go mod replace

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@micbar micbar self-requested a review October 7, 2021 08:03
@butonic butonic removed the request for review from micbar October 7, 2021 08:03
Copy link
Member

@micbar micbar left a comment

Choose a reason for hiding this comment

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

I checked it too, LGTM

Copy link
Contributor

@ishank011 ishank011 left a comment

Choose a reason for hiding this comment

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

@butonic looks pretty good, can you just add back the NotSupported error handling of updating the display name in gateway/UpdateReceivedOCMShare and gateway/UpdateReceivedShare?

@ishank011
Copy link
Contributor

Ah okay. You call that mount_point now. My bad :D

@ishank011 ishank011 merged commit 0e6d415 into cs3org:master Oct 7, 2021
@butonic butonic deleted the sharemanager-api-change branch October 7, 2021 13:24
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