Skip to content

New mount setting: read only mount - #36397

Merged
phil-davis merged 3 commits into
masterfrom
read_only_mount
Jan 2, 2020
Merged

New mount setting: read only mount#36397
phil-davis merged 3 commits into
masterfrom
read_only_mount

Conversation

@mmattel

@mmattel mmattel commented Nov 10, 2019

Copy link
Copy Markdown
Contributor

This is an update to #33035 which was closed in favour of the ownCloud 10 codebase.
The original PR was filed by @TJHeeringa

Description

Provide an option in the mount settings for setting a mount to be read only

Related Issue

Motivation and Context

Provide a mount in read only mode.

How Has This Been Tested?

Local tests. Create a mount in master, switch to branch, read only is false. Set the option read only either on an existing mount or a new mount provides the mount in read only mode. Sync client syncs and tells on local changes that you do not have permission to write to.

Screenshots (if appropriate):

image
image

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:

@codecov

codecov Bot commented Nov 10, 2019

Copy link
Copy Markdown

Codecov Report

Merging #36397 into master will decrease coverage by <.01%.
The diff coverage is 57.14%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36397      +/-   ##
============================================
- Coverage     64.68%   64.68%   -0.01%     
- Complexity    19105    19106       +1     
============================================
  Files          1269     1269              
  Lines         74721    74728       +7     
  Branches       1320     1320              
============================================
+ Hits          48333    48337       +4     
- Misses        26000    26003       +3     
  Partials        388      388
Flag Coverage Δ Complexity Δ
#javascript 54.12% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.85% <57.14%> (-0.01%) 19106 <0> (+1)
Impacted Files Coverage Δ Complexity Δ
apps/files_external/js/settings.js 51.84% <ø> (ø) 0 <0> (ø) ⬇️
apps/files_external/lib/Command/ListCommand.php 68.04% <0%> (-0.41%) 0 <0> (ø)
apps/files_external/templates/settings.php 33.69% <100%> (+0.72%) 0 <0> (ø) ⬇️
lib/private/legacy/util.php 73.59% <60%> (-0.12%) 219 <0> (+1)

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 b7cae93...2a19481. Read the comment docs.

@micbar

micbar commented Nov 11, 2019

Copy link
Copy Markdown
Contributor

@mmattel I restarted the CI.
Let see, what happens.

I would be glad if we could cover this with tests.

@mmattel

mmattel commented Nov 12, 2019

Copy link
Copy Markdown
Contributor Author

@micbar there are only two tests I found using setMountOptions and getMountOptions but none of them are tests manipulating file data...

@DeepDiver1975 DeepDiver1975 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

THX @mmattel

  • squash commits
  • use a descriptive commit message - the PR title would work
@micbar

micbar commented Nov 15, 2019

Copy link
Copy Markdown
Contributor

@mmattel THX for the Changelog :-)

@mmattel

mmattel commented Nov 15, 2019

Copy link
Copy Markdown
Contributor Author

@DeepDiver1975 squashed and rebased

@mmattel

mmattel commented Nov 15, 2019

Copy link
Copy Markdown
Contributor Author

@jvillafanez mind merging?

@micbar

micbar commented Nov 15, 2019

Copy link
Copy Markdown
Contributor

@jvillafanez mind merging?

You need @DeepDiver1975 to approve, he requested the changes.

@mmattel

mmattel commented Nov 19, 2019

Copy link
Copy Markdown
Contributor Author

Ping?

@phil-davis

Copy link
Copy Markdown
Contributor

@DeepDiver1975 ping for re-review.
@micbar schedule for release? If so, then can have some acceptance tests added.

@mmattel

mmattel commented Dec 19, 2019

Copy link
Copy Markdown
Contributor Author
@mmattel

mmattel commented Dec 19, 2019

Copy link
Copy Markdown
Contributor Author

@pmaier1 @micbar In the same way I agreed my contribution/maintenance in the other PR, I do the same here. Hope this helps to proceed.

@phil-davis I would appreciate your assistance for the tests

@phil-davis phil-davis self-assigned this Dec 30, 2019
@phil-davis
phil-davis dismissed DeepDiver1975’s stale review December 30, 2019 08:27

review comments have been addressed

@phil-davis

phil-davis commented Dec 30, 2019

Copy link
Copy Markdown
Contributor

@individual-it plese review the test that I added (latest commit)

Comment thread tests/acceptance/features/webUIAdminSettings/adminStorageSettings.feature Outdated
Comment thread tests/acceptance/features/bootstrap/WebUIAdminStorageSettingsContext.php Outdated
@phil-davis
phil-davis dismissed individual-it’s stale review December 30, 2019 12:43

review comments have been addressed

@phil-davis

Copy link
Copy Markdown
Contributor

@individual-it ready for review of tests again
I added lots of lines to the Then section of the scenarios to check what download, upload, rename, delete can or cannot be done.

@phil-davis

Copy link
Copy Markdown
Contributor

@micbar this is the other PR from @mmattel @TJHeeringa that is a waiting feature.
I have added an acceptance test.
IMO this is "good to go". There has been code review, and @individual-it is doing a final review of my test code.

OK to also get this in 10.4 ?

@individual-it individual-it left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

acceptance tests look good

@phil-davis
phil-davis merged commit 2e58817 into master Jan 2, 2020
@delete-merged-branch
delete-merged-branch Bot deleted the read_only_mount branch January 2, 2020 08:00
@phil-davis

phil-davis commented Jan 2, 2020

Copy link
Copy Markdown
Contributor

I merged to master because the code and tests have both been reviewed, this is wanted, and we are editing acceptance test code in a similar area for other stuff.

I have made a "backport" PR #36680 to 10.4 release branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

6 participants