X Tutup
Skip to content

[For 10.4] Fix "files:transfer-ownership" in S3 multibucket setups#36464

Merged
micbar merged 2 commits intomasterfrom
objectstore_transfer2
Dec 5, 2019
Merged

[For 10.4] Fix "files:transfer-ownership" in S3 multibucket setups#36464
micbar merged 2 commits intomasterfrom
objectstore_transfer2

Conversation

@jvillafanez
Copy link
Member

@jvillafanez jvillafanez commented Nov 22, 2019 •
edited by phil-davis
Loading

Description

Supersedes #36326 , which was having problems and was reverted (in #36460)

Related Issue

https://github.com/owncloud/enterprise/issues/3520
#36266

Motivation and Context

Fix the original issue as well as additional problems found (which caused the revert)

How Has This Been Tested?

occ files:transfer-ownership still works with shares without problems.
In addition, checked the following scenario which was failing with the previous PR:

  1. Create 2 users and make sure both users use different S3 buckets (in a multibucket setup)
  2. user1 creates a folder "foo1" and shares it with user2
  3. user1 uploads a file in his root folder
  4. user1 moves the uploaded file into the shared folder ("file1.txt" -> "foo1/file1.txt")
  5. Check that the file is moved without a problem and that user 2 can access to the shared file.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@jvillafanez jvillafanez self-assigned this Nov 22, 2019
@codecov
Copy link

codecov bot commented Nov 22, 2019 •
edited
Loading

Codecov Report

Merging #36464 into master will decrease coverage by 0.6%.
The diff coverage is 47.27%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36464      +/-   ##
============================================
- Coverage     64.66%   64.05%   -0.61%     
- Complexity    19025    19036      +11     
============================================
  Files          1268     1268              
  Lines         74398    74395       -3     
  Branches       1311     1311              
============================================
- Hits          48108    47654     -454     
- Misses        25904    26355     +451     
  Partials        386      386
Flag Coverage Δ Complexity Δ
#javascript 54.02% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.16% <47.27%> (-0.68%) 19036 <2> (+11)
Impacted Files Coverage Δ Complexity Δ
...b/private/Files/ObjectStore/ObjectStoreStorage.php 66.2% <47.27%> (-17.52%) 105 <2> (+11)
lib/private/DB/PostgreSqlMigrator.php 0% <0%> (-100%) 4% <0%> (ø)
...ilder/ExpressionBuilder/PgSqlExpressionBuilder.php 0% <0%> (-88.89%) 3% <0%> (ø)
lib/private/DB/AdapterOCI8.php 0% <0%> (-86.67%) 4% <0%> (ø)
...ivate/Files/ObjectStore/HomeObjectStoreStorage.php 0% <0%> (-86.67%) 8% <0%> (ø)
lib/private/DB/AdapterPgSql.php 0% <0%> (-85.72%) 2% <0%> (ø)
...Builder/ExpressionBuilder/OCIExpressionBuilder.php 0% <0%> (-85.19%) 18% <0%> (ø)
lib/private/DB/OracleMigrator.php 0% <0%> (-76.85%) 10% <0%> (ø)
lib/private/DB/OracleConnection.php 0% <0%> (-66.67%) 12% <0%> (ø)
lib/private/Files/Storage/Wrapper/Availability.php 10.38% <0%> (-40.26%) 80% <0%> (ø)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fae369...a5215bd. Read the comment docs.

@phil-davis
Copy link
Contributor

phil-davis commented Nov 22, 2019 •
edited
Loading

To test against this with files_primary_s3 we will also need to revert the markTestSkippedIfMultiBucketObjectStorage() changes, so that the test actually runs in object storage.

Update: drone CI was green here, so I pushed Revert "Skip testTransferAllFiles when on ObjectStoreStorage" so I can try from files_primary_s3

@phil-davis phil-davis force-pushed the objectstore_transfer2 branch from 2661df2 to d3fd56e Compare November 22, 2019 14:59
@micbar
Copy link
Contributor

micbar commented Nov 24, 2019

To test against this with files_primary_s3 we will also need to revert the markTestSkippedIfMultiBucketObjectStorage() changes, so that the test actually runs in object storage.

Update: drone CI was green here, so I pushed Revert "Skip testTransferAllFiles when on ObjectStoreStorage" so I can try from files_primary_s3

Ok, sounds like progress.

@phil-davis phil-davis mentioned this pull request Nov 25, 2019
Closed
@phil-davis
Copy link
Contributor

phil-davis commented Nov 25, 2019

owncloud/files_primary_s3#271 (comment)
All 9899 core unit tests are passing - great start!

@phil-davis phil-davis force-pushed the objectstore_transfer2 branch from d3fd56e to 93a0364 Compare November 26, 2019 04:06
@phil-davis
Copy link
Contributor

phil-davis commented Nov 26, 2019

@jvillafanez I rebased this just now so that it has all the things that happened in core master related to getting files_primary_s3 CI green.

Now I will run this from files_primary_s3 and see the result.

@phil-davis phil-davis mentioned this pull request Nov 26, 2019
Closed
@jvillafanez
Copy link
Member Author

jvillafanez commented Nov 28, 2019

Moving to review because the tests pass and there is no additional development planned (unless bugs happen)

@jvillafanez jvillafanez changed the title [WIP] Objectstore transfer2 Objectstore transfer2 Nov 28, 2019
micbar
micbar suggested changes Nov 28, 2019
View reviewed changes
Copy link
Contributor

@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.

needs changelog item

karakayasemi
karakayasemi approved these changes Nov 28, 2019
View reviewed changes
Copy link
Contributor

@karakayasemi karakayasemi left a comment

Choose a reason for hiding this comment

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

The code looks good. I just added some small suggestions that caught my attention while reading.

@jvillafanez jvillafanez force-pushed the objectstore_transfer2 branch from 0073430 to a6b4bb5 Compare November 29, 2019 09:59
@phil-davis
Copy link
Contributor

phil-davis commented Dec 2, 2019

@micbar IMO this should be OK for 10.4 - if so, please add it to the 10.4 project.
Then we can rebase it (to be sure!) and I will run the files_primary_s3 CI against it again to check that all is well.

@micbar micbar added this to the development milestone Dec 2, 2019
@phil-davis phil-davis force-pushed the objectstore_transfer2 branch from a6b4bb5 to a5215bd Compare December 5, 2019 11:16
@phil-davis phil-davis changed the title Objectstore transfer2 [For 10.4] Fix "files:transfer-ownership" in S3 multibucket setups Dec 5, 2019
@phil-davis
Copy link
Contributor

phil-davis commented Dec 5, 2019

I rebased and squashed the commits just now. Let's see what CI says here in core, and in owncloud/files_primary_s3#271 (which I just pushed again to run this core code with files_primary_s3)

@phil-davis
Copy link
Contributor

phil-davis commented Dec 5, 2019

CI passed in owncloud/files_primary_s3#271 so this is good from a QA PoV.
@micbar feel free to override the codecov result and merge if it is OK with you.

@micbar micbar merged commit 6e72e01 into master Dec 5, 2019
@delete-merged-branch delete-merged-branch bot deleted the objectstore_transfer2 branch December 5, 2019 14:39
@davitol davitol mentioned this pull request Jan 9, 2020
Closed
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phil-davis phil-davis phil-davis approved these changes

@VicDeo VicDeo Awaiting requested review from VicDeo

+2 more reviewers

@micbar micbar micbar approved these changes

@karakayasemi karakayasemi karakayasemi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jvillafanez jvillafanez

Labels

4 - To release

Projects

None yet

Milestone

development

Development

Successfully merging this pull request may close these issues.

4 participants

@jvillafanez @phil-davis @micbar @karakayasemi
X Tutup