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

New mount setting: read only mount #36397

Merged
merged 3 commits into from
Jan 2, 2020
Merged

New mount setting: read only mount #36397

merged 3 commits into from
Jan 2, 2020

Conversation

mmattel
Copy link
Contributor

@mmattel mmattel commented Nov 10, 2019

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

codecov bot commented Nov 10, 2019

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

micbar commented Nov 11, 2019

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

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

@mmattel
Copy link
Contributor Author

mmattel commented Nov 12, 2019

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

Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

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

micbar commented Nov 15, 2019

@mmattel THX for the Changelog :-)

@mmattel
Copy link
Contributor Author

mmattel commented Nov 15, 2019

@DeepDiver1975 squashed and rebased

@mmattel
Copy link
Contributor Author

mmattel commented Nov 15, 2019

@jvillafanez mind merging?

@micbar
Copy link
Contributor

micbar commented Nov 15, 2019

@jvillafanez mind merging?

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

@mmattel
Copy link
Contributor Author

mmattel commented Nov 19, 2019

Ping?

@phil-davis
Copy link
Contributor

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

@mmattel
Copy link
Contributor Author

mmattel commented Dec 19, 2019

@DeepDiver1975 @micbar ping?

@mmattel
Copy link
Contributor Author

mmattel commented Dec 19, 2019

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

phil-davis commented Dec 30, 2019

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

@phil-davis phil-davis dismissed individual-it’s stale review December 30, 2019 12:43

review comments have been addressed

@phil-davis
Copy link
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
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 ?

Copy link
Member

@individual-it individual-it left a comment

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

phil-davis commented Jan 2, 2020

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Mark mount as read only in the mount settings
6 participants