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

occ config:system:set can now set other value types #18444

Merged
merged 4 commits into from
Jan 25, 2016

Conversation

RobinMcCorkell
Copy link
Member

Integers, doubles, booleans and even arrays can now be set, with the --type=... option. Array setting can be specified by passing multiple name arguments, e.g. ./occ config:system:set redis port --value=123 --type=integer

Fixes #17687

cc @nickvergessen @DeepDiver1975 @MorrisJobke

@scrutinizer-notifier
Copy link

A new inspection was created.

@nickvergessen
Copy link
Contributor

How do you do nested arrays? See oobjecttstore array in the sample config

@RobinMcCorkell
Copy link
Member Author

./occ config:system:set objectstore arguments autocreate --value=true --type=bool

@MorrisJobke MorrisJobke added this to the 8.2-current milestone Aug 20, 2015
@nickvergessen
Copy link
Contributor

@Xenopathic well int, bool and null is nice!

But array still lacks nesting support. Please post a command that writes the following to the config:
https://github.com/owncloud/core/blob/master/config/config.sample.php#L898-L920

@RobinMcCorkell
Copy link
Member Author

@nickvergessen It would be next to impossible to get a command-line syntax to set an entire array at once, that's why I decided to treat array values as subsections almost. So you set each sub-value individually:

./occ config:system:set objectstore class --value='OC\\Files\\ObjectStore\\Swift'
./occ config:system:set objectstore arguments username --value='facebook100000123456789'
./occ config:system:set objectstore arguments password --value='Secr3tPaSSWoRdt7'
./occ config:system:set objectstore arguments container --value='owncloud'
./occ config:system:set objectstore arguments autocreate --value=true --type=bool
./occ config:system:set objectstore arguments region --value='RegionOne'
./occ config:system:set objectstore arguments url --value='http://8.21.28.222:5000/v2.0'
./occ config:system:set objectstore arguments tenantName --value='facebook100000123456789'
./occ config:system:set objectstore arguments serviceName --value='swift'

Imagine trying to set that in a single command line?

@nickvergessen
Copy link
Contributor

yeah well, I guess that works as well. Should have that on delete too then?

The only other idea I could imagine would be json_encode(), but I guess that's too difficult to achieve, so your solution seems better, just needs proper documentation adjustments:
https://github.com/owncloud/documentation/blob/master/admin_manual/configuration_server/occ_command.rst#config-commands

@RobinMcCorkell
Copy link
Member Author

Well, TBH I think all 3 commands (get, set, delete) should be merged into one command. The way it would work is like this:

  • ./occ config:system option returns the value of the config parameter 'option'
  • ./occ config:system option --set=foobar sets the value of the config parameter 'option' to 'foobar'
  • ./occ config:system option --delete deletes the config parameter 'option'

For an example that is already in master, see the new ./occ log:manage command: https://github.com/owncloud/core/blob/master/core/command/log/manage.php. It uses the system I just described above (without delete of course, that doesn't make sense for log configuration).

I could implement that in this PR if you'd like? Then another PR to move config:app over too?

@RobinMcCorkell RobinMcCorkell removed this from the 8.2-current milestone Sep 8, 2015
@RobinMcCorkell
Copy link
Member Author

Perhaps not for 8.2 😄

@MorrisJobke
Copy link
Contributor

@cmonteroluque @DeepDiver1975 Do we want this in 8.2? On the one hand the code is ready - on the other hand it is only a sev3 ticket and an enhancement.

case 'b':
$value = strtolower($value);
switch ($value) {
case 'true':
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong intendation

@RobinMcCorkell
Copy link
Member Author

@MorrisJobke Also, this brings inconsistencies between config:system:set and the other config:system:* commands. I proposed merging all the commands into a central config:system (with compatibility for the then-legacy commands) but it was never implemented...

@MorrisJobke MorrisJobke added this to the 9.0-next milestone Sep 30, 2015
@ghost
Copy link

ghost commented Sep 30, 2015

@Xenopathic @MorrisJobke cool

@PVince81
Copy link
Contributor

The problem now is that if we decide to change the behavior or remove commands, these need to be deprecated first, because some admins might have already started using these in scripts. So it would qualify as public API.

@nickvergessen
Copy link
Contributor

  • Rebased
  • Restored BC for set
  • Added nested support to delete
  • Added nested support to get

Robin McCorkell and others added 4 commits January 14, 2016 15:02
Integers, doubles, booleans and even arrays can now be set, with the
--type=... option. Array setting can be specified by passing multiple
name arguments, e.g. `./occ config:system:set redis port --value=123
--type=integer`
@nickvergessen
Copy link
Contributor

@Xenopathic @MorrisJobke please reviews

@LukasReschke
Copy link
Member

Works 👍

@MorrisJobke
Copy link
Contributor

Tested and works 👍

DeepDiver1975 added a commit that referenced this pull request Jan 25, 2016
occ config:system:set can now set other value types
@DeepDiver1975 DeepDiver1975 merged commit ce75323 into master Jan 25, 2016
@DeepDiver1975 DeepDiver1975 deleted the occ-config-types branch January 25, 2016 09:02
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
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.

occ config:system:set does not allow to set arrays, integer and booleans
7 participants