Skip to content

s3 external storage fixes #29220

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.

Sign up for GitHub

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

Jump to bottom
Merged
skjnldsv merged 11 commits into from
Oct 22, 2021
Merged

s3 external storage fixes #29220

skjnldsv merged 11 commits into from
Oct 22, 2021

Conversation

@icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Oct 13, 2021 •
edited
Loading

  • Fix "delete markers" causing ghost folders
  • More efficient file scanning
  • Make directory copy a bit more reliable
  • More reliable detection of changed folder contents
  • CI against 2 different "s3 compatible" implementations and with versioning enabled and disabled

@icewind1991 icewind1991 added the 2. developing Work in progress label Oct 13, 2021
@icewind1991 icewind1991 marked this pull request as draft October 13, 2021 17:43
@icewind1991 icewind1991 force-pushed the s3-external-list branch 10 times, most recently from a851448 to 3eed59d Compare October 14, 2021 18:00
@icewind1991 icewind1991 changed the title s3 external storage listing rework s3 external storage fixes Oct 15, 2021
@icewind1991
s3 external storage listing rework
09ffac5 
Signed-off-by: Robin Appelman <[email protected]>
@icewind1991
doesDirectoryExist fixes
5e3c8b3 
Signed-off-by: Robin Appelman <[email protected]>
@icewind1991
more reliable directory copy
294af42 
Signed-off-by: Robin Appelman <[email protected]>
@icewind1991
ci
294b218 
Signed-off-by: Robin Appelman <[email protected]>
@icewind1991
optimize filetype for s3 directories a bit
d3bd0b5 
Signed-off-by: Robin Appelman <[email protected]>
@icewind1991
remove old migration method
3463769 
Signed-off-by: Robin Appelman <[email protected]>
@icewind1991
more reliable hasUpdated for s3
4bd08af 
Signed-off-by: Robin Appelman <[email protected]>
@icewind1991
more reliable return value for Watcher::checkUpdate
55346b5 
Signed-off-by: Robin Appelman <[email protected]>
@icewind1991
Copy link
Member Author

icewind1991 commented Oct 15, 2021

Ok, this should be good now.

It grew a bit in scope outside the "fix ghost folders" in order to have ci be happy in all cases but those changes should be good to have anyway

@icewind1991 icewind1991 marked this pull request as ready for review October 15, 2021 15:19
@icewind1991 icewind1991 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 15, 2021
@icewind1991 icewind1991 requested a review from PVince81 October 15, 2021 15:19
@icewind1991 icewind1991 added this to the Nextcloud 23 milestone Oct 15, 2021
@PVince81 PVince81 mentioned this pull request Oct 18, 2021
Closed
PVince81
PVince81 reviewed Oct 18, 2021
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

I've tested the main cases like rename, move and also create a folder with a similar name for #25370

All worked fine. Well done!

See comments

@PVince81 PVince81 mentioned this pull request Oct 18, 2021
Closed
PVince81
PVince81 reviewed Oct 21, 2021
View reviewed changes
apps/files_external/lib/Lib/Storage/AmazonS3.php
/** @var bool|null */
private $versioningEnabled = null;

/** @var IMemcache */
Copy link
Member

@PVince81 PVince81 Oct 21, 2021

Choose a reason for hiding this comment

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

make that ICache ?

PVince81
PVince81 approved these changes Oct 21, 2021
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

mind having a look at the code scanning issues before merging ?

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish bug and removed 3. to review Waiting for reviews labels Oct 22, 2021
@skjnldsv skjnldsv merged commit 2be0eda into master Oct 22, 2021
@skjnldsv skjnldsv deleted the s3-external-list branch October 22, 2021 10:06
@PVince81
Copy link
Member

PVince81 commented Oct 22, 2021

should we limit backport of this to stable22 ?
it's a rather big impactful change

@skjnldsv
Copy link
Member

skjnldsv commented Oct 22, 2021

Ok! 👍

@skjnldsv
Copy link
Member

skjnldsv commented Oct 22, 2021

/backport to stable22

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Oct 22, 2021
Merged
@skjnldsv skjnldsv mentioned this pull request Oct 25, 2021
Merged
25 tasks
@icewind1991
Copy link
Member Author

icewind1991 commented Oct 26, 2021

/backport to stable21

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Oct 26, 2021
Merged
@CarlSchwan CarlSchwan mentioned this pull request Oct 29, 2021
Closed
This was referenced Nov 2, 2021
Closed
Closed
@PVince81 PVince81 mentioned this pull request Feb 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PVince81 PVince81 PVince81 approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

Assignees

No one assigned

Labels

4. to release Ready to be released and/or waiting for tests to finish bug

Projects

None yet

Milestone

Nextcloud 23

Development

Successfully merging this pull request may close these issues.

4 participants

@icewind1991 @PVince81 @skjnldsv