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

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

Merged
merged 2 commits into from
Dec 5, 2019

Conversation

jvillafanez
Copy link
Member

@jvillafanez jvillafanez commented Nov 22, 2019

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

@codecov
Copy link

codecov bot commented Nov 22, 2019

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

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
Copy link
Contributor

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

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

@jvillafanez
Copy link
Member Author

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

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

@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

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants