Add ability to enable/disable maintenance mode from cli. #8748

scolebrook · 2014-05-27T17:08:53Z

Simple addition to the capabilities of console.php to turn maintenance mode on or off. Basically a dupe of the singleuser.php script by Robin Appelman. Useful for backup scripts so that database and data directory snapshots can be taken with no changes from users. If these two things are not perfectly in sync when backed up, restore will be a major headache.

karlitschek · 2014-05-27T17:30:28Z

I was under the impression that we have this already. But obviously not. :-) 👍
@icewind1991

ghost · 2014-05-27T18:55:24Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5066/

DeepDiver1975 · 2014-05-27T20:53:24Z

core/command/maintenance/mmode.php

+use Symfony\Component\Console\Input\InputOption;
+use Symfony\Component\Console\Output\OutputInterface;
+
+class MMode extends Command {


Why not just Mode?

DeepDiver1975 · 2014-05-27T20:54:46Z

besides the noted MMode/mmode: 👍

DeepDiver1975 · 2014-05-27T20:56:27Z

core/command/maintenance/mmode.php

+
+	protected function execute(InputInterface $input, OutputInterface $output) {
+		if ($input->getOption('on')) {
+			\OC_Config::setValue('maintenance', true);


I would prefer if the legacy class OC_Config would not be used - but this has to be addressed separately

icewind1991 · 2014-05-28T10:58:08Z

Looks good besides the MMode thing already mentioned

scolebrook · 2014-05-28T13:39:26Z

I picked mmode because the existing name of maintenance:singleuser for single user mode would mean maintenance:maintenance for consistency. Too long and redundant. I thought about using just mode. Can't remember exactly why I settled on mmode. I agree mode would be better. I'll switch it.

@DeepDiver1975 I haven't dug into what is and isn't legacy before. I see the lib/private/legacy folder now. What is the correct way to use \OC\Config? I see that OC_Config is a wrapper for it with base.php making an OC\Config instance for OC_Config. Everywhere I can see OC\Config used I see OC_Config being referenced to get that instance (eg in core/register_command.php when instantiating the ConvertType command class). This looks like an area that is half way through refactoring.

DeepDiver1975 · 2014-05-28T13:58:32Z

This looks like an area that is half way through refactoring.

exactly - and because of that we can ignore this within this pr

scolebrook · 2014-05-28T14:14:23Z

Consider it ignored :-)

bantu · 2014-05-28T17:13:48Z

I'm not sure what sort of refactoring you are talking about, but you really should take the Config object as a constructor dependency, just like $application->add(new OC\Core\Command\Db\ConvertType(OC_Config::getObject(), new \OC\DB\ConnectionFactory()));.

bantu · 2014-05-28T17:15:49Z

core/command/maintenance/mode.php

+			\OC_Config::setValue('maintenance', false);
+			$output->writeln('Maintenance mode disabled');
+		} else {
+			if (\OC_Config::getValue('maintenance', false)) {


Use elseif instead, which is one level less deep.

From a computation perspective it doesn't make any difference but I prefer this style. There are three available options, on, off and nothing. In the event of nothing, status is displayed which can have two possible values.

So determining and returning that status is nested. Representing the 3 options as 4 parts of the same if block because one of the options can have two return values makes it a little less readable to my mind.

bantu · 2014-05-28T17:19:04Z

Consider using <info> for success messages. See http://symfony.com/doc/current/components/console/introduction.html#coloring-the-output

bantu · 2014-05-28T17:20:36Z

core/command/maintenance/mode.php

@@ -0,0 +1,52 @@
+<?php
+/**
+ * Copyright (c) 2013 Robin Appelman <icewind@owncloud.com>


Most of the code was written in 2013 by Robin. This is just a cp of singleuser.php with a handful of changes for the different config option. So I left it. Should it be changed to 2014 or 2013-2014?

Add another copyright line with 2014 and your name?

Assuming that you probably copied singleuser.php to mode.php and then swapped all the logic contained in the actually command class, I am not sure there is anything remaining that would have to attributed to robin.

bantu · 2014-05-28T17:22:12Z

I am not sure using the options on and off is such a good idea. A mandatory argument seems to be the better choice to me.

ghost · 2014-05-28T18:05:13Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5110/

scolebrook · 2014-05-28T19:46:09Z

The refactoring I was talking about was how OC_Config is legacy but is the only way to get the OC\Config object that is instantiated in base.php. The more I think about it the more I think following your suggestion and using OC\Config inside the class is the better way to go. register_command.php will still need to be updated when there is an alternate way to get that object but at least the class won't. Since mode.php is basically a copy of singleuser.php it would be good to keep them in sync. Anyone have any objections or comments on changing this for both files?

bantu · 2014-05-28T19:50:56Z

Since mode.php is basically a copy of singleuser.php it would be good to keep them in sync. Anyone have any objections or comments on changing this for both files?

Please use a new pull request for that. These are different issues. You are adding something new, so use the knowledge that is currently available. I am also pretty sure I complained about lack of dependency injection in the PR for singleuser.php. If you change singleuser.php here, this PR will take longer to get merged.

scolebrook · 2014-05-28T20:06:41Z

Yes, hadn't thought of DI for this. Good point.

scrutinizer-notifier · 2014-05-28T20:26:25Z

A new inspection was created.

bantu · 2014-05-28T20:34:40Z

👍 This looks good to me. Will test and merge as soon as Jenkins passes.

scolebrook · 2014-05-28T20:39:06Z

Cool.

ghost · 2014-05-29T00:32:01Z

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/5132/

Add ability to enable/disable maintenance mode from cli. * owncloud/cli_maintenance_mode: Use OC\Config instead of OC_Config Changed class name to mode Add ability to enable/disable maintenance mode from cli.

bantu · 2014-05-29T01:53:24Z

Tested as follows

afischer@leonard:/home/www-data/projects/owncloud/core (master) $ sudo su -s /bin/bash -c "./occ maintenance:mode" www-data
Maintenance mode is currently disabled
afischer@leonard:/home/www-data/projects/owncloud/core (master) $ sudo su -s /bin/bash -c "./occ maintenance:mode --help" www-data
Usage:
 maintenance:mode [--on] [--off]

Options:
 --on                  enable maintenance mode
 --off                 disable maintenance mode
 --help (-h)           Display this help message.
 --quiet (-q)          Do not output any message.
 --verbose (-v|vv|vvv) Increase the verbosity of messages: 1 for normal output, 2 for more verbose output and 3 for debug
 --version (-V)        Display this application version.
 --ansi                Force ANSI output.
 --no-ansi             Disable ANSI output.
 --no-interaction (-n) Do not ask any interactive question.

afischer@leonard:/home/www-data/projects/owncloud/core (master) $ sudo su -s /bin/bash -c "./occ maintenance:mode --on --off" www-data
Maintenance mode enabled
afischer@leonard:/home/www-data/projects/owncloud/core (master) $ sudo cat config/config.php | grep main
  'maintenance' => true,
  'trusted_domains' => 
afischer@leonard:/home/www-data/projects/owncloud/core (master) $ sudo su -s /bin/bash -c "./occ maintenance:mode --off" www-data
Maintenance mode disabled
afischer@leonard:/home/www-data/projects/owncloud/core (master) $ sudo cat config/config.php | grep main
  'maintenance' => false,
  'trusted_domains' =>

Add ability to enable/disable maintenance mode from cli.

7151369

DeepDiver1975 reviewed May 27, 2014
View reviewed changes

Changed class name to mode

00f811f

bantu reviewed May 28, 2014
View reviewed changes

bantu self-assigned this May 28, 2014

Use OC\Config instead of OC_Config

43759f1

bantu merged commit 43759f1 into master May 29, 2014

bantu deleted the cli_maintenance_mode branch May 29, 2014 01:53

lock bot locked as resolved and limited conversation to collaborators Aug 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to enable/disable maintenance mode from cli. #8748

Add ability to enable/disable maintenance mode from cli. #8748

scolebrook commented May 27, 2014

karlitschek commented May 27, 2014

ghost commented May 27, 2014

DeepDiver1975 May 27, 2014

DeepDiver1975 commented May 27, 2014

DeepDiver1975 May 27, 2014

icewind1991 commented May 28, 2014

scolebrook commented May 28, 2014

DeepDiver1975 commented May 28, 2014

scolebrook commented May 28, 2014

bantu commented May 28, 2014

bantu May 28, 2014

scolebrook May 28, 2014

bantu commented May 28, 2014

bantu May 28, 2014

scolebrook May 28, 2014

bantu May 28, 2014

bantu May 28, 2014

bantu commented May 28, 2014

ghost commented May 28, 2014

scolebrook commented May 28, 2014

bantu commented May 28, 2014

scolebrook commented May 28, 2014

scrutinizer-notifier commented May 28, 2014

bantu commented May 28, 2014

scolebrook commented May 28, 2014

ghost commented May 29, 2014

bantu commented May 29, 2014

Add ability to enable/disable maintenance mode from cli. #8748

Add ability to enable/disable maintenance mode from cli. #8748

Conversation

scolebrook commented May 27, 2014

karlitschek commented May 27, 2014

ghost commented May 27, 2014

Choose a reason for hiding this comment

DeepDiver1975 commented May 27, 2014

Choose a reason for hiding this comment

icewind1991 commented May 28, 2014

scolebrook commented May 28, 2014

DeepDiver1975 commented May 28, 2014

scolebrook commented May 28, 2014

bantu commented May 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bantu commented May 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bantu commented May 28, 2014

ghost commented May 28, 2014

scolebrook commented May 28, 2014

bantu commented May 28, 2014

scolebrook commented May 28, 2014

scrutinizer-notifier commented May 28, 2014

bantu commented May 28, 2014

scolebrook commented May 28, 2014

ghost commented May 29, 2014

bantu commented May 29, 2014