-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
don't move files in cache twice, fixes renaming for objectstores #17641
Conversation
A new inspection was created. |
fixes #15702 |
hmmm .. behavioral change with no adoption of unit tests? This is fishy - please investigate - THX |
👎 until impact/unit testing solved - THX |
For what is is worth I have applied this change in the lab and it works as expected (files and folders no longer disappear on rename)...hope this is helpful!!! |
@butonic @icewind1991 Can you have a look at this please |
Patch applied in my owncloud installation using public openstack swift service as storage and now it works. I hope to see this branch merged as soon as possible. |
96e7e19
to
2575f64
Compare
superseded by #19414 which contains this fix as well as a testsuite |
Reopening because the testsuite is now in place. @DeepDiver1975 please enable objectstore tests for this one |
pinging @icewind1991 and @MorrisJobke for review now that the testsuite is in place. |
done |
Change looks good 👍 |
TODO:
|
2575f64
to
ba3a529
Compare
@icewind1991 keep it coming! This is awesome! |
SQLite seems to fail:
|
c4e7feb
to
fed3994
Compare
rebased ... |
Nice job @icewind1991 🚀 |
I'm actually setting the milestone to 9.0 since this pr is against master. @karlitschek we need to backport this to fix serious issues in 8.2.1 |
agreed. please backport |
The unit tests crash on a btrfs docker host ...
@butonic Is there a way to fix this easily? Otherwise I'm fine with this but can't test it. But code wise this looks good 👍 |
@MorrisJobke hm did you run autotest or just start the container? It needs to run in privileged mode so that loopback devices can be used. Another solution would be to autodetect btrfs and then not do the whole loobpack device magic as docker can then use the native snapshots to do the fs layering. You could try ripping the loopback stuff from https://github.com/owncloud/core/blob/master/tests/objectstore/entrypoint.sh to see if that helps. Another thing is that the test currently uses a 300MB storage ... which is enough to run the test suite but might not be enough to test large file uploads. |
I did:
I will try to add a condition for btrfs ... but I think this can be hard to detect 🙈 |
I see no need in spending more time into this - @butonic already invested days in finally funding a running solution for our ci system - this is enough from my pov at the moment. |
Run |
Already solved: #19864 |
👍 Awesome work icewind! Thanks for taking a look at this! |
don't move files in cache twice, fixes renaming for objectstores
@butonic can you create the backport? Thanks |
will create PRs for 8.2 and then try 8.1 |
up to now we did only talk about 8.2 - what is the need to have this in 8.1? |
probably none with the next enterprise edition being based on 8.2. Let's see who starts screaming first... |
cc @icewind1991