/juju

Specify a default cloud name in lxd provider #6735

Merged
merged 2 commits into from Jan 4, 2017

Conversation

Reviewers

@jameinel jameinel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@babbageclunk @jameinel @jujubot
@babbageclunk
Member

babbageclunk commented Dec 20, 2016

This means that even if the controller is bootstrapped using the
provider name "lxd" rather than the built-in cloud name "localhost", the
cloud will be created with the name localhost. Migrating between controllers
where one was bootstrapped with "lxd" and the other with "localhost" works.

Fixes https://bugs.launchpad.net/juju/+bug/1650251

QA steps:

  • Bootstrap one controller specifying localhost as the cloud.
  • Bootstrap another with lxd.
  • Migrate a model from one to the other.
  • The model should be successfully migrated.
@babbageclunk
Member

babbageclunk commented Dec 20, 2016

!!Chittychitty!!
@jameinel

jameinel reviewed Dec 20, 2016
View changes

I have a small niggle about what the function looks like, thoughts?

cmd/juju/commands/bootstrap.go
@@ -598,7 +607,7 @@ func (c *bootstrapCommand) handleCommandLineErrorsAndInfoRequests(ctx *cmd.Conte
return false, nil
}
-func (c *bootstrapCommand) cloud(ctx *cmd.Context) (*jujucloud.Cloud, error) {
+func (c *bootstrapCommand) cloud(ctx *cmd.Context) (string, *jujucloud.Cloud, error) {
@jameinel

jameinel Dec 20, 2016

Owner

It seems a little odd that "cloudName" takes precedence over the Cloud object.

Also, I probably would have thought you would do
cloud.Name()
at some later time, rather than having this 'cloud' object do the indirection to find the preferred default name.
(or possibly DefaultCloudName(cloudObject))
Then you could even just do:
c.Cloud = DefaultCloudName(cloudObject)
and the other code wouldn't have to change?

@babbageclunk

babbageclunk Dec 20, 2016 •
Edited 1 time

Member

Do you mean that the name gets returned first? To me it reads like returning the name and the value - that's why I picked that ordering.

I thought about adding Name to Cloud. This makes the most sense to me, but it has implications for serialisation that I'm not happy about - should I also add Name to the internal cloud type for marshalling? I don't want to change the format of clouds.yaml. Adding Name.Cloud but then only populating it on the way through this function seems confusing too.

I toyed with just changing c.Cloud but decided it was nasty to mutate the bootstrapCommand as a side-effect of returning a Cloud, which is why I returned the name instead.

babbageclunk added some commits Dec 20, 2016

@babbageclunk
Specify a default cloud name in lxd provider 
This means that even if the controller is bootstrapped using the
provider name "lxd" rather than the built-in cloud name "localhost", the
cloud will be created with the name localhost. Migrating between these
controllers will work.

Fixes https://bugs.launchpad.net/juju/+bug/1650251
15e3120
@babbageclunk
Add Cloud.Name instead of handling name separately 
Don't store the name in the file, it's redundant - when the cloud is
loaded populate the name from the key in the enclosing map.
3915b59
@babbageclunk
Member

babbageclunk commented Dec 22, 2016

Added Cloud.Name as suggested by @jameinel and @perrito666 - you guys were right, it's much cleaner.

!!chittychitty!!
@jameinel
Owner

jameinel commented Jan 4, 2017

LGTM
@babbageclunk
Member

babbageclunk commented Jan 4, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Jan 4, 2017

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

@babbageclunk babbageclunk referenced this pull request Jan 4, 2017

Merged

Specify a default cloud name in lxd provider (backport) #6760

@jujubot
Contributor

jujubot commented Jan 4, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/9941
@babbageclunk
Member

babbageclunk commented Jan 4, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Jan 4, 2017

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

jujubot added a commit that referenced this pull request Jan 4, 2017

@jujubot
Merge pull request #6760 from babbageclunk/mm-default-cloud-name-2.1 
Specify a default cloud name in lxd provider (backport)

(Backport of #6735 to 2.1 branch.)

This means that even if the controller is bootstrapped using the
provider name "lxd" rather than the built-in cloud name "localhost", the
cloud will be created with the name localhost. Migrating between controllers
where one was bootstrapped with "lxd" and the other with "localhost" works.

Fixes https://bugs.launchpad.net/juju/+bug/1650251

QA steps:

Bootstrap one controller specifying localhost as the cloud.
Bootstrap another with lxd.
Migrate a model from one to the other.
The model should be successfully migrated.
6c2ce39

@jujubot jujubot merged commit 4f7074b into juju:develop Jan 4, 2017

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