Add "occ config:*" to manage config values #16368

nickvergessen · 2015-05-15T11:11:29Z

When we are able to set/import configs via a console command, this can be added to scripts. So in the end, people don't need to use a second.config.php to publish configs when setting up the same owncloud on multiple servers. In the end this makes it possible, to start getting rid of the writable config.php file, which causes problems from time to time since ever. Ideally only a minimum set of configuration values is in the config.php. Especially any values that are/can be changed via the web UI should not be written to the file.

TODO

Add a command to export/list the existing configs (system + apps):

./occ config:list --help
Usage:
 config:list [--output[="..."]] [--public] [app]

Arguments:
 app                   Name of the app ("system" to get the config.php values, "all" for all apps and system) (default: "all")

Options:
 --output              Output format (plain, json or json_pretty, default is plain) (default: "plain")
 --public              Use this option when you want to exclude sensitive configs like passwords, salts, ...

Add a command to import configs (system + apps)
Can either be a file on the system we are importing, or ./occ config:import < admin-mashine-file

./occ config:import --help
Usage:
 config:import [file]

Arguments:
 file                  File with the json array to import

Add command to set a system config value
Add command to get a system config value
Add command to delete a system config value
Add command to set an app config value
Add command to get an app config value
Add command to delete an app config value
Adjust issue template similar to Add occ app:list and ldap:show-config to issue_template.md #16846

nickvergessen · 2015-05-15T11:13:03Z

core/command/config/import.php

+ * @throws \UnexpectedValueException when the array is invalid
+ */
+ protected function getArrayFromFile($importFile) {
+ $helo = file_get_contents($importFile);


Would be nice to be able to use occ config:import < file.json instead of reading from the filesystem of the owncloud instance. But I failed to understand if that works and how.

See https://github.com/owncloud/core/blob/master/core/command/db/converttype.php#L147

What is $helo?

nickvergessen · 2015-07-03T07:04:14Z

Rebased, ready to review @MorrisJobke @DeepDiver1975 @LukasReschke

ghost · 2015-07-03T08:39:14Z

🚀 Test PASSed.🚀

PVince81 · 2015-07-06T13:32:13Z

I'd suggest to reverse the default: by default do not output the "private" attributes (like the public mode) and only output them if the admin explicitly asks for them. This would align with the way the LDAP config exporter works, see https://github.com/owncloud/core/blob/master/apps/user_ldap/command/showconfig.php#L56

Also I guess this here could obsolete the LDAP specific config commands ? @blizzz

PVince81 · 2015-07-06T13:36:49Z

I'd prefer to be able to set a value directly using:
occ config:app:set files_locking enabled no
instead of:
occ config:app:set files_locking enabled --value=no
instead of having to add --value (usually such arguments are rather for optional args)
Should we protect the special attributes of apps like version, enabled ? (ok, version might be good to trigger updates)

PVince81 · 2015-07-06T13:40:51Z

Would be cool to have commands config:system without args that does the same as config:list but only for system. Same with a config:app command. For CLI tools I usually like it when I can "build up" the command by typing more, for example:
- ./occ config:system => show me a list of system values, so I pick one
- ./occ config:system logtimezone => show me the value of that argument
- ./occ config:system logtimezone --setvalue="CEST". => change the value

In the current workflow I'd need to go back and re-edit the left part of the command to add ":get" or ":set" after I saw it.
Anyway, that's just some thought.
Being able to list the values with ./occ config:system should already be enough.

PVince81 · 2015-07-06T13:44:58Z

We could even push this further and only allow the existing config values for system to avoid setting non-existing values. That would require having a catalog of supported config switches. The same for apps, if they were able to enumerate supported config switches. Probably something for much later.

PVince81 · 2015-07-06T13:46:21Z

And now again I types ./occ config:app:get files_locking expecting it to give me all switches for files_locking so I could export that app's config, but it didn't. (admin UX problem 😉)

nickvergessen · 2015-07-06T13:47:31Z

I'd prefer to be able to set a value directly using:
occ config:app:set files_locking enabled no
instead of:
occ config:app:set files_locking enabled --value=no
instead of having to add --value (usually such arguments are rather for optional args)

While this would be nice, we need special treatment for empty strings. so I figured instead of starting two code paths, we can just always use --value

Should we protect the special attributes of apps like version, enabled ? (ok, version might be good to trigger updates)

For the *:set we could, but I don't really think we should do that. you can screw it up with import anyway and you can also change stuff in the db directly. occ is risky, you need to know what you are doing...

...

About building the command, the problem is this merges all stuff into one command, ending up in a huge unmaintainable block of code, so sorry if the :set is annoying to you, but I think it's better to have it explicit.

We could even push this further and only allow the existing config values for system to avoid setting non-existing values. That would require having a catalog of supported config switches. The same for apps, if they were able to enumerate supported config switches. Probably something for much later.

We don't do such stuff and we really shouldn't. Maybe you want to set the values before enabling the app, etc. there are a lot of usecases for having it arbitrary.

PVince81 · 2015-07-06T13:47:55Z

the export function doesn't write the app name in json: occ config:list files_locking --output json > locking.json gives {"enabled":"no","installed_version":"","no":null,"types":"filesystem"}. This means I have to remember which app it was from if I wanted to re-import it.

nickvergessen · 2015-07-06T13:48:45Z

@PVince81 you can not import a single app atm, only the full thing, although that sounds like something we could/should fix

PVince81 · 2015-07-06T13:49:09Z

My comments were all from my own usability testing. Would be good to know what other think to.
I'm fine with keeping the commands as is.

PVince81 · 2015-07-06T13:54:12Z

core/command/config/app/getconfig.php

+
+ $this
+ ->setName('config:app:get')
+ ->setDescription('Set an app config value')


PVince81 · 2015-07-06T13:55:45Z

About building the command, the problem is this merges all stuff into one command, ending up in a huge unmaintainable block of code

This wouldn't be an issue. The command only be the entry point and call functions in separate classes for separate functionality. But I understand that you might not want to make bigger changes at this point.

PVince81 · 2015-07-06T13:56:14Z

Apart from #16368 (comment) I'm fine for merging this.

karlitschek · 2015-07-06T14:59:56Z

I love it 👍

karlitschek · 2015-07-06T15:00:19Z

But needs good documentation. @nickvergessen @carlaschroder

blizzz · 2015-07-07T08:49:13Z

Also I guess this here could obsolete the LDAP specific config commands ? @blizzz

The LDAP config commands are slightly more sophisticated. In first line because there the configkey contains the config ID for the configuration plus the actual config name. So the commands split it up and let the user just specify the which config they want to see, change or delete.

Another thing: in LDAP we accept multiline values in few cases and output them differently in occ (;-separated) and accept them ;-seperated which is dealt with in the app's set method. This could be resolved in a generic way, though.

Also the --public (yes, better make it --private, #16368 (comment)) switch is not effective for apps.

So, it could obsolete them, but would cost convenience. Would not do it at this stage.

blizzz · 2015-07-07T08:57:08Z

I feel not most comfortable with circumventing apps' set methods in general (or disable/enable as @PVince81 pointed out already), while I acknowledge the convenience and helpfulness of this command. Yes, you could change stuff in the DB directly, nevertheless this really lowers the hurdle to do so. For the greater benefit: 👍

DeepDiver1975 · 2015-07-07T09:00:48Z

@nickvergessen can you plase rebase this before you leave? THX

nickvergessen · 2015-07-07T09:33:25Z

Rebased, fixed the docs typo, inverted the "public" to "private" and made it possible to import config:list system and config:list <app>

scrutinizer-notifier · 2015-07-07T09:33:36Z

A new inspection was created.

ghost · 2015-07-07T11:24:15Z

🚀 Test PASSed.🚀

PVince81 · 2015-07-07T12:49:21Z

👍

Add "occ config:*" to manage config values

carlaschroder · 2015-07-07T14:17:03Z

Holy cow that's a lot of work! I can hear occ breathing.

nickvergessen · 2015-07-07T14:17:54Z

@carlaschroder I will do that for you

carlaschroder · 2015-07-07T14:20:16Z

@nickvergessen I meant that's a lot of work on occ. It looks great.

nickvergessen added enhancement 2 - Developing technical debt labels May 15, 2015

nickvergessen self-assigned this May 15, 2015

nickvergessen added this to the 8.2-next milestone May 15, 2015

nickvergessen reviewed May 15, 2015
View reviewed changes

nickvergessen force-pushed the occ-config-commands branch from 6bf1c55 to 34913f6 Compare June 9, 2015 13:11

nickvergessen added 3 - To Review and removed 2 - Developing labels Jul 3, 2015

nickvergessen force-pushed the occ-config-commands branch from 9edf255 to 0d0e102 Compare July 3, 2015 07:03

PVince81 reviewed Jul 6, 2015
View reviewed changes

core/command/config/app/getconfig.php

$this

->setName('config:app:get')

->setDescription('Set an app config value')

Copy link

Contributor

PVince81 Jul 6, 2015

Choose a reason for hiding this comment

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

=> "Get"

Sort registration of commands alphabetically

6d4cb1b

nickvergessen added 17 commits July 7, 2015 11:18

Add a command to list existing configs

6763637

Add an option to exclude sensitive values (e.g. for reports)

2f65332

Missing doc

5d84876

Fix no value

371a4d0

Add tests for list config

16907f9

Split the system from the app config

5370bba

Add a command to set a system config value

a83eac3

Add a command to get a config value

3608c10

Add a command to delete a system config

bc084c4

Add a command to import an json array into the config

86952f8

Allow reading from stdin

130b5d8

Add commands to get, set and delete app config values

2b86eb0

Replace deprecated method calls

d2ca3a2

Add example command to the issue_template.md

f09fe56

Fix typo in command description

9710608

Make it possible to import exportet app/system configs

e5bc9d0

Invert the "public" to a "private" method

85f0125

nickvergessen force-pushed the occ-config-commands branch from 0d0e102 to 85f0125 Compare July 7, 2015 09:33

PVince81 pushed a commit that referenced this pull request Jul 7, 2015

Merge pull request #16368 from owncloud/occ-config-commands

d3e26a3

Add "occ config:*" to manage config values

PVince81 merged commit d3e26a3 into master Jul 7, 2015

PVince81 deleted the occ-config-commands branch July 7, 2015 12:49

carlaschroder mentioned this pull request Jul 7, 2015

new occ commands owncloud-archive/documentation#1292

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "occ config:*" to manage config values #16368

Add "occ config:*" to manage config values #16368

nickvergessen commented May 15, 2015

nickvergessen May 15, 2015

bantu May 19, 2015

bantu May 19, 2015

nickvergessen commented Jul 3, 2015

ghost commented Jul 3, 2015

PVince81 commented Jul 6, 2015

PVince81 commented Jul 6, 2015

PVince81 commented Jul 6, 2015

PVince81 commented Jul 6, 2015

PVince81 commented Jul 6, 2015

nickvergessen commented Jul 6, 2015

PVince81 commented Jul 6, 2015

nickvergessen commented Jul 6, 2015

PVince81 commented Jul 6, 2015

PVince81 Jul 6, 2015

PVince81 commented Jul 6, 2015

PVince81 commented Jul 6, 2015

karlitschek commented Jul 6, 2015

karlitschek commented Jul 6, 2015

blizzz commented Jul 7, 2015

blizzz commented Jul 7, 2015

DeepDiver1975 commented Jul 7, 2015

nickvergessen commented Jul 7, 2015

scrutinizer-notifier commented Jul 7, 2015

ghost commented Jul 7, 2015

PVince81 commented Jul 7, 2015

carlaschroder commented Jul 7, 2015

nickvergessen commented Jul 7, 2015

carlaschroder commented Jul 7, 2015

Add "occ config:*" to manage config values #16368

Add "occ config:*" to manage config values #16368

Conversation

nickvergessen commented May 15, 2015

TODO

nickvergessen May 15, 2015

Choose a reason for hiding this comment

bantu May 19, 2015

Choose a reason for hiding this comment

bantu May 19, 2015

Choose a reason for hiding this comment

nickvergessen commented Jul 3, 2015

ghost commented Jul 3, 2015

PVince81 commented Jul 6, 2015

PVince81 commented Jul 6, 2015

PVince81 commented Jul 6, 2015

PVince81 commented Jul 6, 2015

PVince81 commented Jul 6, 2015

nickvergessen commented Jul 6, 2015

PVince81 commented Jul 6, 2015

nickvergessen commented Jul 6, 2015

PVince81 commented Jul 6, 2015

PVince81 Jul 6, 2015

Choose a reason for hiding this comment

PVince81 commented Jul 6, 2015

PVince81 commented Jul 6, 2015

karlitschek commented Jul 6, 2015

karlitschek commented Jul 6, 2015

blizzz commented Jul 7, 2015

blizzz commented Jul 7, 2015

DeepDiver1975 commented Jul 7, 2015

nickvergessen commented Jul 7, 2015

scrutinizer-notifier commented Jul 7, 2015

ghost commented Jul 7, 2015

PVince81 commented Jul 7, 2015

carlaschroder commented Jul 7, 2015

nickvergessen commented Jul 7, 2015

carlaschroder commented Jul 7, 2015