Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
feature/add region to model defaults cli #6308
Conversation
axw
requested changes
Sep 23, 2016
The documentation text for model-defaults needs updating.
I feel like the logic for Init could be simplified, e.g. moving some of the validation for mixed positional args earlier, so we can get consistent error messages. The tests are fairly comprehensive, so I could live with that coming later.
| @@ -8,12 +8,18 @@ import ( | ||
| "sort" | ||
| "strings" | ||
| + "gopkg.in/juju/names.v2" | ||
| + |
| + key string | ||
| + resetKeys []string | ||
| + cloudName, regionName string | ||
| + // reset holds the contents of the reset flag. It contains a list of comma |
axw
Sep 23, 2016
•
Edited 1 time
-
axw
Sep 23, 2016
Member
-
Sep 23, 2016
This comment is misleading. It's only true until after Init is called, when we call parseResetKeys.
reedobrien
Sep 23, 2016
•
Edited 1 time
-
reedobrien
Sep 23, 2016
Contributor
-
Sep 23, 2016
I'll modify it. I am fine with removing many of the comments. I mostly wrote them out to rubber duck the logistics. Maybe they're useful to future readers or maybe confusing.
| } | ||
| // Init implements part of the cmd.Command interface. | ||
| +// Abandon all hope ye who enter here.... | ||
| +// This needs to parse the a command line invocation as defined in | ||
| +// https://goo.gl/yqrrPI. |
axw
Sep 23, 2016
Member
I think you should drop the doc reference. It's Canonical-only at the moment, though doesn't seem like it needs to be. It's not really adding anything beyond what you've described here AFAICT.
| +// Abandon all hope ye who enter here.... | ||
| +// This needs to parse the a command line invocation as defined in | ||
| +// https://goo.gl/yqrrPI. | ||
| +// So the following 3 lines are all equivalent: |
axw
Sep 23, 2016
•
Edited 1 time
-
axw
Sep 23, 2016
Member
-
Sep 23, 2016
Can we just state that flags may be interspersed, and leave it at that? You've stated "are equivalent" in numerous examples below, where I think saying it once would be sufficient.
| +// This needs to parse the a command line invocation as defined in | ||
| +// https://goo.gl/yqrrPI. | ||
| +// So the following 3 lines are all equivalent: | ||
| +// juju model-defaults aws/us-east-1 foo=baz --reset bar |
| +// model-defaults shows the settings for a value at all locations that it has a | ||
| +// default set -- or at a minimum the default and "-" for a controller with no | ||
| +// value set. | ||
| +// simultaneously. |
| +// juju model-defaults | ||
| +// juju model-defaults no-proxy | ||
| +// | ||
| +// It is not valid to reset and get or to set and get values. It is also neiter |
| + | ||
| + // Look at the first positional arg and test to see if it is a valid | ||
| + // optional specification of cloud/region or region. If it is then | ||
| + // cloudName and regionName or only regionName are set on the object and |
axw
Sep 23, 2016
Member
This needs amending. It's always both or neither, never just regionName. You're setting c.cloudName to the name of the default cloud.
reedobrien
Sep 23, 2016
Contributor
Fixed. I had originally thought that api filled in the cloud along the way, but learned that was an incorrect assumption and failed to update the documentation.
| + // We want to get settings for the provided key. | ||
| + return c.handleOneArg(args[0]) | ||
| + default: // case args > 1 | ||
| + // Specifying any positional args after a key=value pair is |
| +// leading or trailing comma. It then verifies that we haven't incorrectly | ||
| +// received any key=value pairs and finally sets the value(s) on c.resetKeys. | ||
| +func (c *defaultsCommand) parseResetKeys() error { | ||
| + if len(c.reset) > 0 { |
axw
Sep 23, 2016
Member
perhaps invert to save a level of indentation?
if len(c.reset) == 0 {
return nil
}
...
| if k == config.AgentVersionKey { | ||
| return errors.Errorf("%q cannot be reset", config.AgentVersionKey) | ||
| } | ||
| + if strings.Contains(k, "=") { | ||
| + return errors.Errorf( | ||
| + `--reset accepts a single key "a" or comma delimited keys "a,b,c", received: %q`, k) |
axw
Sep 23, 2016
•
Edited 1 time
-
axw
Sep 23, 2016
Member
-
Sep 23, 2016
it suffices to say just "accepts a comma delimited set of keys". a single key need not be delimited
| + valid, err := c.validCloudRegion(cloud, region) |
axw
Sep 23, 2016
•
Edited 1 time
-
axw
Sep 23, 2016
Member
-
Sep 23, 2016
I think we can avoid a relatively expensive API call if region contains an "=". e.g. if the user runs "juju model-defaults foo=bar". Technically we don't disallow "=" in region names, but I think we should.
reedobrien
Sep 23, 2016
•
Edited 1 time
-
reedobrien
Sep 23, 2016
Contributor
-
Sep 23, 2016
Created a bug: http://pad.lv/1627162 made a note and added a short circuit return if region contains "=".
| + return false, errors.Trace(err) | ||
| + } | ||
| + } else { | ||
| + cTag = names.NewCloudTag(cloudName) |
axw
Sep 23, 2016
Member
Please check names.IsValidCloud here. NewCloudTag will panic if it's invalid.
reedobrien
Sep 23, 2016
Contributor
Thanks I didn't know that. Seems a bad idea to panic though. Shouldn't it
- normalize and return a tag,
- also return an error,
- or be name MustNewCloudTag?
Added a check and early return if the name is not a valid cloud name.
| + | ||
| +// parseSetKeys iterates over the args and make sure that the key=value pairs | ||
| +// are valid. It also checks that the same key isn't also being reset. | ||
| +func (c *defaultsCommand) parseSetKeys(args []string) error { |
axw
Sep 23, 2016
Member
can you please move this below handleSetArgs? flows a bit better if the thing using it comes first.
reedobrien
Sep 23, 2016
Contributor
I had grouped the parse and handle methods together, but moved below as requested.
| + return errors.Trace(err) | ||
| + } | ||
| + | ||
| + keyCounter := map[string]int{} |
axw
Sep 23, 2016
Member
keyCounter is unnecessary. keyvalues.Parse already checks for duplicates, and you only care about existence, not count
| + keyCounter[k]++ | ||
| + c.values[k] = v | ||
| + } | ||
| + for _, resetKey := range c.resetKeys { |
axw
Sep 23, 2016
Member
for _, k := range c.resetKeys {
if _, ok := c.values[k]; ok {
return errors.Errorf("key %q cannot be both set and unset in the same command", k)
}
}
?
| + // It doesn't make sense to specify a region unless we are resetting | ||
| + // values here. | ||
| + return errors.New("specifying a region when retrieving defaults is invalid") | ||
| + case regionSpecified && resetSpecified: |
| - return client, nil | ||
| +// handleOneArg handles the case where we have one positional arg after | ||
| +// processing for a region and the reset flag after processing for a region and |
| +func (c *defaultsCommand) handleOneArg(arg string) error { | ||
| + resetSpecified := c.resetKeys != nil | ||
| + regionSpecified := c.regionName != "" | ||
| + |
axw
Sep 23, 2016
Member
I'm finding this truth-table style to be not particularly helpful. If you structure as if/else, then you don't have to keep all of the variables in your head for each case. You can then short-circuit the conditions. YMMV, perhaps it's just me.
if regionSpecified {
if !resetSpecified {
return errors.New("specifying a region when retrieving defaults for a setting is invalid")
}
return errors.New("cannot retrieve defaults for a key and reset args at the same time")
}
if resetSpecified {
return errors.Errorf("invalid region specified: %q", arg)
}
c.key = arg
c.action = c.getDefaults
return nil
reedobrien
Sep 23, 2016
•
Edited 1 time
-
reedobrien
Sep 24, 2016
Contributor
-
Sep 24, 2016
Edit: Since I essentially agreed I decided to just edit the switches with multiple variables per case.
| + return errors.New("specifying a region when retrieving defaults for a setting is invalid") | ||
| + case !regionSpecified && resetSpecified: | ||
| + // It makes no sense to supply a positional arg that isn't a region if | ||
| + // we are resetting a region, so we must have gotten an invalid region. |
| + // already. | ||
| + return errors.New("cannot retrieve defaults for a key and reset args at the same time") | ||
| + default: | ||
| + // This should be unreachable. |
axw
Sep 23, 2016
Member
This is unreachable. You've got a truth table on 2 booleans, with the 4 cases. If you use if/else you can get rid of this :)
| + switch { | ||
| + case numArgs == 2 && !regionSpecified && resetSpecified: | ||
| + // It makes no sense to supply a positional arg that isn't a region if | ||
| + // we are resetting a region, so we must have gotten an invalid region. |
| - } | ||
| + // allKeys gets the all the set and reset keys to validate. This should get | ||
| + // caught in Init so maybe we don't need it here too. | ||
| + // TODO(reviewer) Should we keep this and the init check? |
axw
Sep 23, 2016
Member
it's still necessary to check that they're known, but not that they're duplicated. also, the functional literal seems unnecessary? wouldn't the following suffice?
allKeys := c.resetKeys[:]
for k := range c.values {
allKeys = append(allKeys, k)
}
| - for _, key := range keys() { | ||
| + | ||
| + keys := allkeys() | ||
| + repeats := map[string]int{} |
| + }, { | ||
| + description: "test invalid positional args with set", | ||
| + args: []string{"a=b", "b", "c=d"}, | ||
| + errorMatch: `expected "key=value", got "b"`, |
reedobrien
Sep 24, 2016
•
Edited 1 time
-
reedobrien
Sep 24, 2016
Contributor
-
Sep 24, 2016
Not sure about that. If the key only is at the end or beginning the user may have thought they were interspersing get and set args, but if the key only is in the middle of set args it's more likely they miss-typed. But that is the problem with many possibilities in a single command:/ The error above IIRC comes from trying to parse the key values with keyvalues.Parse. The error below comes because we knew it was not part of the key values.
Are you saying we should check for the above error and return the below error?
axw
Sep 26, 2016
Member
Are you saying we should check for the above error and return the below error?
I'm thinking we should check for mixed get/set up-front, before we call keyvalues.Parse.
| @@ -95,16 +230,16 @@ func (s *DefaultsCommandSuite) TestDefaultsInit(c *gc.C) { | ||
| } | ||
axw
Sep 23, 2016
Member
please also add tests that verify that the correct cloud/region are passed through to the API calls
| + settings, err = createSettings(st, globalSettingsC, key, attrs) | ||
| + if err != nil { | ||
| + return errors.Trace(err) | ||
| + } |
reedobrien
Sep 24, 2016
•
Edited 1 time
-
reedobrien
Sep 24, 2016
Contributor
-
Sep 24, 2016
Yeah. Thought about it, thanks for the push. Updated.
| + err = s.State.UpdateModelConfigDefaultValues(attrs, nil, rspec) | ||
| + c.Assert(err, jc.ErrorIsNil) | ||
| + | ||
| + // Then check in another state. |
| + // It doesn't make sense to specify a region unless we are resetting | ||
| + // values here. | ||
| + return errors.New("specifying a region when retrieving defaults is invalid") | ||
| + case regionSpecified && resetSpecified: |
| + return errors.New("specifying a region when retrieving defaults for a setting is invalid") | ||
| + case !regionSpecified && resetSpecified: | ||
| + // It makes no sense to supply a positional arg that isn't a region if | ||
| + // we are resetting a region, so we must have gotten an invalid region. |
| + // already. | ||
| + return errors.New("cannot retrieve defaults for a key and reset args at the same time") | ||
| + default: | ||
| + // This should be unreachable. |
axw
Sep 23, 2016
Member
This is unreachable. You've got a truth table on 2 booleans, with the 4 cases. If you use if/else you can get rid of this :)
| + switch { | ||
| + case numArgs == 2 && !regionSpecified && resetSpecified: | ||
| + // It makes no sense to supply a positional arg that isn't a region if | ||
| + // we are resetting a region, so we must have gotten an invalid region. |
| - } | ||
| + // allKeys gets the all the set and reset keys to validate. This should get | ||
| + // caught in Init so maybe we don't need it here too. | ||
| + // TODO(reviewer) Should we keep this and the init check? |
axw
Sep 23, 2016
Member
it's still necessary to check that they're known, but not that they're duplicated. also, the functional literal seems unnecessary? wouldn't the following suffice?
allKeys := c.resetKeys[:]
for k := range c.values {
allKeys = append(allKeys, k)
}
| - for _, key := range keys() { | ||
| + | ||
| + keys := allkeys() | ||
| + repeats := map[string]int{} |
| + }, { | ||
| + description: "test invalid positional args with set", | ||
| + args: []string{"a=b", "b", "c=d"}, | ||
| + errorMatch: `expected "key=value", got "b"`, |
reedobrien
Sep 24, 2016
•
Edited 1 time
-
reedobrien
Sep 24, 2016
Contributor
-
Sep 24, 2016
Not sure about that. If the key only is at the end or beginning the user may have thought they were interspersing get and set args, but if the key only is in the middle of set args it's more likely they miss-typed. But that is the problem with many possibilities in a single command:/ The error above IIRC comes from trying to parse the key values with keyvalues.Parse. The error below comes because we knew it was not part of the key values.
Are you saying we should check for the above error and return the below error?
axw
Sep 26, 2016
Member
Are you saying we should check for the above error and return the below error?
I'm thinking we should check for mixed get/set up-front, before we call keyvalues.Parse.
| @@ -95,16 +230,16 @@ func (s *DefaultsCommandSuite) TestDefaultsInit(c *gc.C) { | ||
| } | ||
axw
Sep 23, 2016
Member
please also add tests that verify that the correct cloud/region are passed through to the API calls
| + settings, err = createSettings(st, globalSettingsC, key, attrs) | ||
| + if err != nil { | ||
| + return errors.Trace(err) | ||
| + } |
reedobrien
Sep 24, 2016
•
Edited 1 time
-
reedobrien
Sep 24, 2016
Contributor
-
Sep 24, 2016
Yeah. Thought about it, thanks for the push. Updated.
| + err = s.State.UpdateModelConfigDefaultValues(attrs, nil, rspec) | ||
| + c.Assert(err, jc.ErrorIsNil) | ||
| + | ||
| + // Then check in another state. |
| -// juju model-defaults aws/us-east-1 foo=baz --reset bar | ||
| -// juju model-defaults aws/us-east-1 --reset bar foo=baz | ||
| -// juju model-defaults --reset bar aws/us-east-1 foo=baz | ||
| +// This needs to parse the a command line invocation to reset and set, or get |
| +// juju model-defaults aws/us-east-1 foo=baz --reset bar | ||
| +// | ||
| +// If aws is the cloud of the current or specified controller -- specified by | ||
| +// -c somecontroller or -m somecontroller:somemodel -- then the following would |
| @@ -292,6 +280,12 @@ func (c *defaultsCommand) parseCloudRegion(args []string) ([]string, error) { | ||
| region = cr | ||
| } | ||
| + // NB(redir) http://pad.lv/1627162 -- We don't disallow "=" in region |
axw
Sep 26, 2016
Member
please use the format
// TODO(redir) YYYY-MM-DD #1627162
// We don't disallow "=" in region names,
// but probably should.
reedobrien
Oct 6, 2016
Contributor
Added tests to show cloud and region are conveys through the call to the api.
reedobrien
added some commits
Oct 6, 2016
|
@axw @wallyworld ready for another look. |
|
$$merge$$ Thanks @axw |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
reedobrien commentedSep 23, 2016
Also changes --reset to be a stringvar. All tests pass at this point but
it needs more tests for the new features. It also adds a lot of command
arg scrutiny and tests
This was originally part of #6269 but has been split from the backend
bits here. Addtionally per review in pr #6269, updates the reset variable
so we can specify it repeatedly and reset all the keys set in all the
reset flags.
Ensure the cloud is always set, even if we only get the region on the
CLI. Finally, create new region settings if they weren't created at
bootstrap.
Refs: config command collapse spec https://goo.gl/yqrrPI
Refs: model-config-tree
QA:
bootstrap a controller with settings similar to
http://paste.ubuntu.com/23080517/
verify the controller val was reset
verify it was reset in us-east-1.
verify both items were set on us-west-2
verify no-proxy was changed and agent-stream reset
verify us-east-1 is set again
be sure the controller default was set again.
error: invalid region specified: "us-east-2" # verify this error.
... Do other things to try breaking it.