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.

Sign up for GitHub

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

Jump to bottom

Handle read only apps folders properly #28543

Merged
merged 1 commit into from
Aug 7, 2017
Merged

Handle read only apps folders properly #28543

merged 1 commit into from
Aug 7, 2017

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Jul 31, 2017 •
edited
Loading

Description

This is planned to support clustered setup much better ....

Related Issue

refs https://github.com/owncloud/platform/issues/133

How Has This Been Tested?

  1. add the config parameter 'cluster.mode' -> true
  2. try to install
  3. try to operate as usual
  4. expectation: nothing will break apart ....

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.

@DeepDiver1975 DeepDiver1975 added this to the development milestone Jul 31, 2017
@DeepDiver1975 DeepDiver1975 requested a review from PVince81 July 31, 2017 14:09
@felixboehm felixboehm added the p1-urgent Critical issue, need to consider hotfix with just that issue label Jul 31, 2017
PVince81
PVince81 reviewed Aug 1, 2017
View reviewed changes
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.

Please add back the check for "config_is_read_only" as upgrading to 10.0.3 would likely cause surprises for people who have it set.

If we want to remove it then deprecate it in 10.0.3 and remove completely in 11.0. But I'm not sure if we should remove it as some people without cluster setup might want to have the config read-only, so they wouldn't think about setting cluster mode.

lib/private/Config.php Outdated
@@ -272,5 +281,12 @@ private function writeData() {
\OC_Util::clearOpcodeCache();
}
}

public function isReadOnly() {
if (!$this->getValue('installed', false)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

also include the config_is_read_only key here ?

lib/private/Installer.php Outdated
@@ -91,7 +91,11 @@ public static function installApp( $data = []) {

$info = self::checkAppsIntegrity($data, $extractDir, $path);
$appId = OC_App::cleanAppId($info['id']);
$basedir = OC_App::getInstallPath().'/'.$appId;
$appsFolder = OC_App::getInstallPath();
if ($appsFolder === null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like you should also add is_writeable and is_readable here ? See https://github.com/owncloud/core/pull/28543/files#diff-ec8c121d705ff47eacbfdd23f36a9f6cL731.

settings/templates/panels/admin/mail.php Outdated
<option value='<?php p($secure)?>' <?php p($selected) ?>><?php p($name) ?></option>
<?php endforeach;?>
</select>
<?php p($l->t('The config file is read only. Please adopt your setup by editing the config file manually.')); ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

adopting a child ?

=> adjust

settings/templates/panels/admin/mail.php Outdated
</select>
<?php p($l->t('The config file is read only. Please adopt your setup by editing the config file manually.')); ?>
<br>
<?php p($l->t('In a clustered setup please make sure to sync the file across all nodes.')); ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

s/the file/the config.php file/

to make sure this is clear

@DeepDiver1975 DeepDiver1975 mentioned this pull request Aug 1, 2017
Merged
@DeepDiver1975
Copy link
Member Author

@PVince81 thanks for your feedback - I removed the config parameter 'cluster.mode' now totally. 'config_is_read_only' will do its job for now

@PVince81
Copy link
Contributor

PVince81 commented Aug 1, 2017

hmm, so does it mean that "config_read_only" will now also block the apps folder ? People who had this set will be surprised that they can't install new apps ?

@DeepDiver1975
Copy link
Member Author

hmm, so does it mean that "config_read_only" will now also block the apps folder ? People who had this set will be surprised that they can't install new apps ?

Thats my point about this all: it's a mess and needs cleanup .... this was the idea in having ONE config switch to rule them all ... 😉

@PVince81
Copy link
Contributor

PVince81 commented Aug 3, 2017

@DeepDiver1975 do we want to surprise these people ? If yes, will need proper communication

@DeepDiver1975
Copy link
Member Author

@DeepDiver1975 do we want to surprise these people ? If yes, will need proper communication

so you advocate for keeping two intendant checks

  • config_is_read_only for config stuff only
  • and test if apps folder is writable

@PVince81
Copy link
Contributor

PVince81 commented Aug 3, 2017

If this was OC 11 I'd be fine with breaking the way it works.

For OC 10 the easiest is just do nothing and maybe add a note about clustering in the documentation about the two settings to change:

  • config_is_read_only
  • the "apps_paths" setting with "writable" set to false

Now if the above doesn't work correctly then we should fix that at least.

@DeepDiver1975 DeepDiver1975 force-pushed the handle-cluster-setup branch 2 times, most recently from 30f9b82 to d93b2c9 Compare August 3, 2017 14:55
@DeepDiver1975
Copy link
Member Author

@PVince81 a much simpler solution now - only addressing if the apps folder is writable

PVince81
PVince81 approved these changes Aug 3, 2017
View reviewed changes
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.

👍 that's better, thanks.

Please rename issue title and merge if you tested this and all still worked fine

@DeepDiver1975 DeepDiver1975 changed the title Introduce cluster.mode Handle read only apps folders properly Aug 3, 2017
@DeepDiver1975 DeepDiver1975 merged commit 8d0bdd6 into master Aug 7, 2017
@DeepDiver1975 DeepDiver1975 deleted the handle-cluster-setup branch August 7, 2017 11:00
@DeepDiver1975 DeepDiver1975 mentioned this pull request Aug 7, 2017
Merged
@lock
Copy link

lock bot commented Aug 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@PVince81 PVince81 PVince81 approved these changes

Assignees

@DeepDiver1975 DeepDiver1975

Labels
4 - To release p1-urgent Critical issue, need to consider hotfix with just that issue status/STALE
Projects
None yet
Milestone
10.0.3
Development

Successfully merging this pull request may close these issues.
4 participants
@DeepDiver1975 @PVince81 @felixboehm @ownclouders