Add region handling to UpdateModelConfigDefaultValues #6297

Merged
merged 2 commits into from Sep 22, 2016

Conversation

Projects
None yet
3 participants
Contributor

reedobrien commented Sep 21, 2016

Refs: model-config-tree

QA: Unit tests all pass. This should have gone in with the rest of the backend model-config region bits. But it didn't it is required for the cli bits to work, so hoping to merge this so we can get feedback on the CLI bits.

axw approved these changes Sep 21, 2016

LGTM, but please don't pass tag strings out of the apiserver layer.

state/modelconfig.go
@@ -134,8 +135,18 @@ func (st *State) modelConfigValues(modelCfg attrValues) (config.ConfigValues, er
}
// UpdateModelConfigDefaultValues updates the inherited settings used when creating a new model.
-func (st *State) UpdateModelConfigDefaultValues(attrs map[string]interface{}, removed []string) error {
- settings, err := readSettings(st, globalSettingsC, controllerInheritedSettingsGlobalKey)
+func (st *State) UpdateModelConfigDefaultValues(attrs map[string]interface{}, removed []string, cloudTag, regionName string) error {
@axw

axw Sep 21, 2016

Member

at this layer, please either pass in a cloud name, or a names.CloudTag

@reedobrien

reedobrien Sep 22, 2016

Contributor

fixed

state/modelconfig.go
+ return errors.Trace(err)
+ }
+ key = regionSettingsGlobalKey(cloud.Id(), regionName)
+ } else {
@axw

axw Sep 21, 2016

Member

return an error if cloudTag != "" && regionName == "" ?

@reedobrien

reedobrien Sep 22, 2016

Contributor

Changed the machinery to use environs.RegionSpec to define the cloud/region and added a constructor that barfs if cloud or region is an empty string.

axw approved these changes Sep 22, 2016

apiserver/modelmanager/modelmanager.go
+ c.state.UpdateModelConfigDefaultValues(nil, arg.Keys, rspec),
+ )
+ } else {
+ results.Results[i].Error = common.ServerError(
@axw

axw Sep 22, 2016

Member

I think it'd be a little neater to de-dupe these two calls, and do the conditional rspec thing like you did in the other method.

@reedobrien

reedobrien Sep 22, 2016

Contributor

Good idea. Fixed. Sqaushing for merge.

reedobrien added some commits Sep 20, 2016

Quick drive-by cleanup of receiver
There were 4 different receiver names for ModelManagerAPI: m, mm, s, c.
Now there is one: m.
Contributor

reedobrien commented Sep 22, 2016

$$merge$$

Contributor

jujubot commented Sep 22, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented Sep 22, 2016

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/9300

Contributor

reedobrien commented Sep 22, 2016

$$merge$$

Contributor

jujubot commented Sep 22, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 982ddb2 into juju:master Sep 22, 2016

@reedobrien reedobrien deleted the reedobrien:feature/region-support-to-state branch Sep 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment