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 move in the owncloud storage driver #1696

Merged
merged 1 commit into from
May 10, 2021

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented May 10, 2021

Update the filepath in redis when moving a file. Didn't implement it in delete since delete is still a bit more broken. But since we don't actively use the owncloud storage driver except for in CI it's not too important.

Fixes #1693

@C0rby C0rby requested a review from labkode as a code owner May 10, 2021 09:38
@C0rby C0rby requested review from butonic and ishank011 May 10, 2021 09:38
@C0rby C0rby force-pushed the fix-move-owncloud-storage branch from 6abb674 to 6b17ec2 Compare May 10, 2021 10:06
@C0rby
Copy link
Contributor Author

C0rby commented May 10, 2021

Argh... cloning the oc10 tests failed...

Cloning into '/drone/src/tmp/testrunner'...
error: RPC failed; curl 56 OpenSSL SSL_read: Connection reset by peer, errno 104
error: 15 bytes of body are still expected
fetch-pack: unexpected disconnect while reading sideband packet
fatal: early EOF
fatal: index-pack failed

I don't have permissions to restart the build...

@C0rby C0rby mentioned this pull request May 10, 2021
@ishank011
Copy link
Contributor

@C0rby how difficult will it be to fix the delete method? Since we're addressing this issue, I'd like for all its occurrences to be fixed because we still have support for the driver in reva. If it'll take too long, then we can skip it

@C0rby
Copy link
Contributor Author

C0rby commented May 10, 2021

The thing about deleting is that purging non empty folders does not work and purging versions of files doesn't work.

I can't really tell how long it will take to fix them.
Not removing the id/path mapping from redis won't break anything. It will just take up some bytes of space.

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.

Okay, but please create an issue about this

// than the acceptance tests we should be fine by ignoring the errors.
_ = filepath.Walk(newIP, func(path string, info os.FileInfo, err error) error {
if err != nil {
log.Error().Str("path", path).Err(err).Msg("error caching id")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a log about rolling back the whole op? And a TODO comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I opened an issue #1699
And added todos.

Update the filepath in redis when moving a file. Didn't implement it in delete since delete is still a bit more broken. But since we don't actively use the owncloud storage driver except for in CI it's not too important.

Fixes cs3org#1693
@C0rby C0rby force-pushed the fix-move-owncloud-storage branch from 6b17ec2 to 0529666 Compare May 10, 2021 14:02
@ishank011 ishank011 merged commit 0fc001d into cs3org:master May 10, 2021
@C0rby C0rby deleted the fix-move-owncloud-storage branch May 10, 2021 14:43
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.

owncloud storage driver doesn't update file path in redis
2 participants