Add per default the apps-external directory in config.php during installation by mmattel · Pull Request #30889 · owncloud/core

mmattel · 2018-03-23T15:56:35Z

Description

When you install the system, a config.php parameter is added to enable the apps-external directory.
An empty apps-external/ directory has been added as part of defaut shipped directories.
Note: if you follow the documentation and run the setup/permisions script, all needed directories including apps-external/ will get correct permissions and ownership.

Related Issue

#29839 ([Feature Request] make the apps-external folder default used in new installations)

Motivation and Context

See the issue description regarding the benefits.

How Has This Been Tested?

Install a new system from scratch.

Screenshots (if appropriate):

  'apps_path' => 
  array (
    0 => 
    array (
      'path' => '/var/www/owncloud/core/apps',
      'url' => '/apps',
      'writable' => 'false',
    ),
    1 => 
    array (
      'path' => '/var/www/owncloud/core/apps-external',
      'url' => '/apps-external',
      'writable' => 'true',
    ),
  ),

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

A documentation PR follows asap.
@settermjd fyi

codecov · 2018-03-23T16:17:32Z

Codecov Report

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

@@             Coverage Diff              @@
##             master   #30889      +/-   ##
============================================
- Coverage     65.17%   65.16%   -0.01%     
- Complexity    18352    18353       +1     
============================================
  Files          1200     1200              
  Lines         69701    69709       +8     
  Branches       1283     1283              
============================================
- Hits          45426    45425       -1     
- Misses        23901    23910       +9     
  Partials        374      374

Flag	Coverage Δ	Complexity Δ
#javascript	`53.1% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.55% <0%> (-0.02%)`	`18353 <0> (+1)`

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Setup.php	`25.66% <0%> (-0.95%)`	`53 <0> (+1)`
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 0783f10...d408ae8. Read the comment docs.

mmattel · 2018-03-24T17:01:25Z

@VicDeo I followed your advice and added the apps2/ directory by default including the .gitkepp file in it + removing the obsolete directory creation logic.

PVince81 · 2018-03-28T08:20:42Z

needs update of "make dist" and "make dist-qa" route to also copy/create the "apps2" folder for inclusion in the release tarballs

mmattel · 2018-03-28T11:10:38Z

@PVince81
What is the name/location of the file to adopt "make dist" and "make dist-qa" ?
Or is this something you do centrally ?

mmattel · 2018-03-28T14:03:14Z

@VicDeo
I added all the changes you requested and sucessfully rerun a test by doing a new install procedure.

mmattel · 2018-03-28T14:32:10Z

@PVince81
I added apps2/ to the Makefile in both sections required (dist and dist-qa) and in core_src_dirs

patrickjahns · 2018-04-03T07:53:53Z

I am very reluctant with this change - it might have implications on:

automated scripts deploying owncloud (ansible, salt etc.)
needs to be tested with our docker containers, since they already use a custom directory for downloaded apps (ref: https://github.com/owncloud-docker/base/blob/master/rootfs/root/owncloud/config.php#L12-L15 )

"apps2" as name is not very descriptive -> might be confusing for people.

mmattel · 2018-04-03T09:48:15Z

@patrickjahns

Regarding automation: Automation and upgrades are exactly the reason for this PR. In a nutshell, if you do not do it, any upgrade is a pain with respect to apps, for more details pls see the text in the referenced issue.
"apps2" as name is not very descriptive: I just reused the name as it was already defined in the documentation section apps and the permission script in documentation not changing things on too many places. If there is the whish to change the directory name, no problem, but we then have to do this in documentation too. My very personal POV, naming apps2 can be improved but is not that bad... One positive thing about naming it apps2 is, that doing a directory listing it is directly after apps.
Regarding Docker: this PR checks if the key apps_path is already present and does nothing in case of preventing overwriting. If the Docker scripts runs after config.php is created, you just need to check if the key exists and do nothing (in the same way as I am doing).

I had a discussion with @PVince81 about the reason for this PR, he maybe can add his opinion.

cdamken · 2018-04-03T12:28:53Z

"apps2" as name is not very descriptive -> might be confusing for people.

we could use apps as the apps that are delivered with the tar file and apps-external as all other apps that are not delivered in the tar file?

That would be helpful for the upgrade just making a copy of that folder to the new ownCloud

mmattel · 2018-04-03T12:37:01Z

I am happy to change the name of apps2 to apps-external if you vote for - just let me know and I proceed.
The location of changes are:

This PR
config.sample.php (there is a PR open with improved text covering the apps_path key anyway)
The documetation (some files)
The Docker file @patrickjahns mentioned

mmattel · 2018-04-06T07:48:42Z

@cdamken
I renamed /apps2 to /apps-external.
If this gets merged, I will update all other documents accordingly.

@patrickjahns
Can you support me with Docker.

tboerger · 2018-04-06T17:11:11Z

Within our docker containers we are writing this config file before the installation: https://github.com/owncloud-docker/base/blob/master/rootfs/root/owncloud/config.php

mmattel · 2018-04-06T17:21:22Z

Thanks for clarification @tboerger
This is good news, because as stated some comments above, this PR does not conflict with an existing key because of checking if exists and do nothing if present.

@VicDeo I did all the changes requested, but github does not recognize one change made and keeps your request open?

ownclouders · 2018-08-30T12:55:56Z

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

patrickjahns · 2018-08-30T12:57:22Z

~~For master this seems reasonable. To change this behavior on stable10 I am not sure.~~
see below

Would also like to have final agreement from @pmaier1 @felixboehm

EDIT

Some more thoughts - with ownCloud 11 we want to move away from a writeable config.php - the intial configuration needs to be read-only.
I am wondering how this concept would fit - as this concept would again require to have a writeable config.php

ownclouders · 2018-08-30T12:57:44Z

Automated rebase with GitMate.io was successful! 🎉

phil-davis · 2018-08-30T13:18:12Z

+					]
+				];
+
+				$config->setSystemValues($defaultAppsPaths);


If we do this, it will be convenient for automated testing, because we will always have specific values of apps_paths in the config - no need for test scenarios to jump through hoops to work out what they should/could be.

If we do this, it will be convenient for automated testing, because we will always have specific values of apps_paths in the config - no need for test scenarios to jump through hoops to work out what they should/could be.

The large goal is moving forward with immutable config.php - and being able to change the apps_path via a command will not work

That will be a challenge for any tests that want to adjust config.php values for testing purposes
I guess when testing remote "black-box" installs, we might anyway skip tests that mess with config.php, since those sort of tests might break the remote system and not (be able to) put it back.
Anyway, a conversation for another place.

mmattel · 2018-08-30T15:02:50Z

@patrickjahns
The basic idea behind having an apps_paths key set by default is, that you can update an existing instance without worrying of apps that have been post installed and are not part of the tar file. As you have two directories, one is part of the tar and fresh installed on each update, the other one contains your installed apps, you just need nothing more than copying the old config.php file to the update and copy/relink apps_external and data directory.

The broader discussion about a design change including config.php is busting this PR.
As the key apps_paths is out for a while and actively used including our docker environment, you have in any case when you redesign config.php management the need of a migration step.

I propose to merge this PR to ease admin life and have the config.php management discussion in a seperate thread.

PVince81 · 2018-10-09T14:33:41Z

php-cs-fixer is not happy

I think we can at least merge this for master / OC 11

mmattel · 2019-02-10T13:12:58Z

I just updated the PR which solved the conflict in Makefile.

Obsolete with updated code

mmattel · 2019-02-18T08:35:42Z

@PVince81 approve? mergable ?

mmattel · 2019-02-19T08:16:37Z

Just squashed the commits

PVince81 · 2019-03-01T15:04:43Z

@mmattel please backport

@phil-davis will this require specific changes in our testing pipeline ? or were you referring to possible future changes we could do ?

VicDeo · 2019-03-01T15:09:25Z

Web updater needs to create this dir if it doesn't exist.

…s2_on_initial_install Add per default the apps-external directory in config.php during installation

phil-davis · 2019-03-02T02:26:34Z

@phil-davis will this require specific changes in our testing pipeline ? or were you referring to possible future changes we could do ?

It just means we can run acceptance tests that check installing an app in the secondary apps path without having to modify config.php - sometimes the system-under-test does not let us modify config.php or it is better that we do not purposely modify it. Issue #34657 raised.

phil-davis · 2019-03-09T02:58:20Z

Backport stable10 #34656

mmattel requested review from PVince81 and cdamken March 23, 2018 15:56

mmattel force-pushed the default_create_apps2_on_initial_install branch from f43a88d to f93c49c Compare March 24, 2018 16:52

This was referenced Mar 26, 2018

Update config.sample.php improving description for key apps_path #30903

Merged

[Feature Request] improvement for update location for shipped apps #30904

Closed

PVince81 added the 3 - To Review label Mar 28, 2018

PVince81 added this to the development milestone Mar 28, 2018

PVince81 assigned mmattel Mar 28, 2018

PVince81 requested a review from DeepDiver1975 March 28, 2018 08:23

VicDeo previously requested changes Mar 28, 2018

View reviewed changes

Comment thread lib/private/Setup.php Outdated

Comment thread lib/private/Setup.php Outdated

Comment thread lib/private/Setup.php Outdated

Comment thread lib/private/Setup.php Outdated

mmattel force-pushed the default_create_apps2_on_initial_install branch from f93c49c to aa82479 Compare March 28, 2018 13:59

mmattel force-pushed the default_create_apps2_on_initial_install branch from aa82479 to 86d6ff5 Compare March 28, 2018 14:27

mmattel force-pushed the default_create_apps2_on_initial_install branch 2 times, most recently from 3fce607 to 374825d Compare April 3, 2018 07:45

patrickjahns self-requested a review April 3, 2018 07:49

mmattel force-pushed the default_create_apps2_on_initial_install branch 2 times, most recently from 65b29ee to 47e1460 Compare April 6, 2018 07:41

ownclouders force-pushed the default_create_apps2_on_initial_install branch from c61ddb6 to 6fccc5b Compare August 30, 2018 12:57

phil-davis reviewed Aug 30, 2018

View reviewed changes

mmattel force-pushed the default_create_apps2_on_initial_install branch from 6fccc5b to c7bc748 Compare February 10, 2019 13:10

mmattel force-pushed the default_create_apps2_on_initial_install branch from c7bc748 to 4b4514a Compare February 15, 2019 15:29

add apps-external during installation

d408ae8

mmattel force-pushed the default_create_apps2_on_initial_install branch from 4b4514a to d408ae8 Compare February 19, 2019 08:16

butonic approved these changes Mar 1, 2019

View reviewed changes

PVince81 merged commit 03e6987 into master Mar 1, 2019

delete-merged-branch Bot deleted the default_create_apps2_on_initial_install branch March 1, 2019 15:04

PVince81 added the backport-request label Mar 1, 2019

mmattel pushed a commit that referenced this pull request Mar 1, 2019

[stable10] Merge pull request #30889 from owncloud/default_create_app…

0431e73

…s2_on_initial_install Add per default the apps-external directory in config.php during installation

mmattel mentioned this pull request Mar 1, 2019

[stable10] Add per default the apps-external directory in config.php during installation #34656

Merged

11 tasks

phil-davis mentioned this pull request Mar 2, 2019

Acceptance tests using pre-installed apps_paths #34657

Closed

3 tasks

phil-davis removed the backport-request label Mar 9, 2019

This was referenced Mar 26, 2019

[Feature Request] make the apps-external folder default used in new installations #29839

Closed

Add the apps-external directory during setup only #34898

Merged

lock Bot locked as resolved and limited conversation to collaborators Mar 10, 2020

Uh oh!

Conversation

mmattel commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

codecov Bot commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mmattel commented Mar 24, 2018

Uh oh!

PVince81 commented Mar 28, 2018

Uh oh!

mmattel commented Mar 28, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmattel commented Mar 28, 2018

Uh oh!

mmattel commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickjahns commented Apr 3, 2018

Uh oh!

mmattel commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdamken commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmattel commented Apr 3, 2018

Uh oh!

mmattel commented Apr 6, 2018

Uh oh!

tboerger commented Apr 6, 2018

Uh oh!

mmattel commented Apr 6, 2018

Uh oh!

ownclouders commented Aug 30, 2018

Uh oh!

patrickjahns commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ownclouders commented Aug 30, 2018

Uh oh!

phil-davis Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

patrickjahns Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

phil-davis Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

mmattel commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Oct 9, 2018

Uh oh!

mmattel commented Feb 10, 2019

Uh oh!

mmattel commented Feb 18, 2019

Uh oh!

mmattel commented Feb 19, 2019

Uh oh!

PVince81 commented Mar 1, 2019

Uh oh!

VicDeo commented Mar 1, 2019

Uh oh!

phil-davis commented Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mmattel commented Mar 23, 2018 •

edited

Loading

codecov Bot commented Mar 23, 2018 •

edited

Loading

mmattel commented Mar 28, 2018 •

edited

Loading

mmattel commented Apr 3, 2018 •

edited

Loading

cdamken commented Apr 3, 2018 •

edited

Loading

patrickjahns commented Aug 30, 2018 •

edited

Loading

mmattel commented Aug 30, 2018 •

edited

Loading

phil-davis commented Mar 2, 2019 •

edited

Loading