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

Add the apps-external directory during setup only #34898

Merged
merged 1 commit into from
Mar 26, 2019
Merged

Conversation

mmattel
Copy link
Contributor

@mmattel mmattel commented Mar 26, 2019

Description

Related Issue

Motivation and Context

Prevents re-adding that directory on every update.

How Has This Been Tested?

  • make test-php-unit TEST_DATABASE=mysql TEST_PHP_SUITE=tests/lib/SetupTest.php --> ok

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:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@PVince81
Copy link
Contributor

PVince81 commented Mar 26, 2019

  • TEST: no op when folder exists and no write permissions
  • TEST: folder creates when not existing
  • TEST: error when no permission when attempting creation
  • TEST: created folder has write permissions

@PVince81
Copy link
Contributor

tested with occ maintenance:install ^

lib/private/Setup.php Outdated Show resolved Hide resolved
lib/private/Setup.php Outdated Show resolved Hide resolved
@PVince81 PVince81 added this to the development milestone Mar 26, 2019
@codecov
Copy link

codecov bot commented Mar 26, 2019

Codecov Report

Merging #34898 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #34898      +/-   ##
============================================
- Coverage     65.35%   65.34%   -0.01%     
- Complexity    18488    18492       +4     
============================================
  Files          1208     1208              
  Lines         69975    69982       +7     
  Branches       1280     1280              
============================================
- Hits          45732    45730       -2     
- Misses        23871    23880       +9     
  Partials        372      372
Flag Coverage Δ Complexity Δ
#javascript 53.04% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.75% <0%> (-0.02%) 18492 <0> (+4)
Impacted Files Coverage Δ Complexity Δ
lib/private/Setup.php 24.89% <0%> (-0.78%) 57 <0> (+4)
apps/files_trashbin/lib/Expiration.php 96.55% <0%> (-1.73%) 29% <0%> (ø)
lib/private/Server.php 86.56% <0%> (-0.12%) 253% <0%> (ø)

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 67c4ba1...304aaf1. Read the comment docs.

@mmattel mmattel force-pushed the apps_external_new branch from 01fb3f0 to 304aaf1 Compare March 26, 2019 14:21
@mmattel
Copy link
Contributor Author

mmattel commented Mar 26, 2019

Command line installation - OK

sudo -uwww-data ./occ maintenance:install --database "mysql" --database-name "apptest" \
--database-host "localhost" --database-user "xyz" --database-pass "abc" \
--admin-user "admin" --admin-pass "def" 
ownCloud was successfully installed

apps-external directory got created

Making an existing apps-external owned by root (=no write access to www-data) - OK
Cleaning up the database and running the same installation command from above
Can't create or write into the apps-external directory /mnt/mm-klaus/www/owncloud/core-test/apps-external

@PVince81
Copy link
Contributor

forgot to say that in my setup the setup goes through but the folder "apps-external" doesn't get created, because the file_exists check is done on the data folder, not on "apps-external" and returns true, so it doesn't attempt to create it.

@mmattel
Copy link
Contributor Author

mmattel commented Mar 26, 2019

I fixed the issue, tested it, squashed and pushed, see my test from above.

@PVince81
Copy link
Contributor

ah ok, didn't see the update. thanks


// create the apps-external directory only during installation
$appsExternalDir = \OC::$SERVERROOT.'/apps-external';
if (!\file_exists($appsExternalDir)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

seems we already do is_dir for existence check and mkdir in the block below, so this one here feel redundant ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just did the same as we do for the data directory to stay inline.
I can of course drop the if statement and just do @mkdir. Just le me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, fine then. it only happens once at setup time, so not a big issue

Copy link
Contributor

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

👍

@PVince81 PVince81 merged commit 22190c9 into master Mar 26, 2019
@delete-merged-branch delete-merged-branch bot deleted the apps_external_new branch March 26, 2019 15:55
@PVince81
Copy link
Contributor

@mmattel please backport

@phil-davis
Copy link
Contributor

Backport stable10 #34902

@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] make the apps-external folder default used in new installations
3 participants