Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Update app config to use --reset as string var #6429
Conversation
| @@ -66,7 +66,8 @@ type configCommand struct { | ||
| applicationName string | ||
| configFile cmd.FileVar | ||
| keys []string | ||
| - reset bool | ||
| + reset []string // Holds the keys to be reset until parsed. |
perrito666
Oct 11, 2016
Contributor
that comment is a doc, just put it on top of the field
// reset holds the keys..
reset []string
reedobrien
Oct 11, 2016
•
Edited 1 time
-
reedobrien
Oct 11, 2016
Contributor
-
Oct 11, 2016
That makes fmt indent all the variables after comments at different levels... plus it is unexported, just a note.
applicationName string
configFile cmd.FileVar
keys []string
// Holds the keys to be reset until parsed.
reset []string
// Holds the keys to be reset once parsed.
resetKeys []string
useFile bool
values attributes
instead of
applicationName string
configFile cmd.FileVar
keys []string
reset []string // Holds the keys to be reset until parsed.
resetKeys []string // Holds the keys to be reset once parsed.
useFile bool
values attributes
| - if len(args) == 0 { | ||
| - return errors.New("no configuration options specified") | ||
| +// parseResetKeys splits the keys provided to --reset after trimming any | ||
| +// leading or trailing comma. It then verifies that we haven't incorrectly |
perrito666
Oct 11, 2016
Contributor
nitpick, this comment seems to be getting too much into implementation.
| + resetKeys = append(resetKeys, keys...) | ||
| + } | ||
| + for _, k := range resetKeys { | ||
| + if strings.Contains(k, "=") { |
reedobrien
Oct 11, 2016
Contributor
This is a good point. We could fail faster and there wouldn't be any loss of fidelity in the error message. However, a PR with this has already landed. I'd say if you think it is important enough we should open a bug and update all three items that now use --reset a,b,c.
| // Run implements the cmd.Command interface. | ||
| func (c *configCommand) Run(ctx *cmd.Context) error { | ||
| client, err := c.getAPI() | ||
| if err != nil { | ||
| return errors.Trace(err) | ||
| } | ||
| defer client.Close() | ||
| + if len(c.resetKeys) > 0 { | ||
| + err := c.resetConfig(client, ctx) |
| + } | ||
| + nv, err := readValue(ctx, v[1:]) | ||
| + if err != nil { | ||
| + return nil, err |
reedobrien
Oct 11, 2016
•
Edited 1 time
-
reedobrien
Oct 11, 2016
Contributor
-
Oct 11, 2016
I just moved that below where it was used because that is someone else's nit:) I didn't read through the existing code carefully though. Nice catch -- fixed :)
| @@ -169,7 +169,19 @@ func (s *configCommandSuite) TestSetCommandInit(c *gc.C) { | ||
| // --reset and no config name provided | ||
| err = coretesting.InitCommand(application.NewConfigCommandForTest(s.fake), []string{"application", "--reset"}) | ||
| - c.Assert(err, gc.ErrorMatches, "no configuration options specified") | ||
| + c.Assert(err, gc.ErrorMatches, "flag needs an argument: --reset") |
perrito666
Oct 11, 2016
Contributor
I might be mistaken but I believe the "can only retrieve one or all... " path is not tested.
reedobrien
Oct 11, 2016
•
Edited 1 time
-
reedobrien
Oct 11, 2016
Contributor
-
Oct 11, 2016
Line 110. I kept the existing test style here, which is a bit different than the other two places where this collapse is happening (and these came from three different commands, too: get, set, unset). Once it lands and it is decided we are going to keep it, we can make a pass at refactoring the three suites to be more congruous.
added a commit
to reedobrien/juju
that referenced
this pull request
Oct 11, 2016
added a commit
to reedobrien/juju
that referenced
this pull request
Oct 11, 2016
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
reedobrien commentedOct 11, 2016
This also allows simultaneous set/reset as with model-defaults and
model-config.
Refs: config command collapse spec https://goo.gl/yqrrPI
QA:
juju config wordpress debug=yes
juju config wordpress debug
"yes"
juju config wordpress --reset debug
juju config wordpress debug
"no"
save the contents of the mysql settings to a file with `juju config mysql --format=yaml > mysql.yaml. It should look like this.
11 Edit the file like this
juju config mysql --file mysql.yaml
for v in size type; do juju config mysql query-cache-$v;done
1.2345679e+08
"ON"
juju config mysql --reset query-cache-size,query-cache-type
for v in size type; do juju config mysql query-cache-$v;done
0
"OFF"
juju config mysql query-cache-size=123456789 query-cache-type=DEMAND
for v in size type; do juju config mysql query-cache-$v;done
1.2345679e+08
"ON"
juju config mysql query-cache-size
1.2345679e+08
juju config mysql --reset query-cache-size
juju config mysql query-cache-size
0
juju config mysql query-cache-type
juju config mysql --reset query-cache-type
juju config mysql query-cache-type
"OFF"
juju config mysql --reset
error: flag needs an argument: --reset
juju config mysql foo bar
error: can only retrieve a single value, or all values
juju kill-controller reed/lxd-app-config -y