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

Default lxd region #9056

Merged
merged 1 commit into from Aug 10, 2018
Merged

Conversation

SimonRichardson
Copy link
Member

Description of change

Update the pollster interaction so that we can have default naming
for object key names. To improve the UI/UX for adding a LXD cloud
interactively we want to state that there can be a region and that
it can be default, we do allow you to override this when adding a
cloud, but a sane default should suffice.

QA steps

juju add-cloud lxd-remote2
Cloud Types
  lxd
  maas
  manual
  oci
  openstack
  oracle
  vsphere

Select cloud type: lxd

Enter the API endpoint url for the remote LXD server: https://10.0.0.225:8443

Auth Types
  certificate
  interactive

Select one or more auth types separated by commas: interactive

Enter region name [use default]: 

Enter the API endpoint url for the region [use cloud api url]: 

Enter another region? (Y/n): n

Cloud "lxd-remote2" successfully added
You may need to `juju add-credential lxd-remote2' if your cloud needs additional credentials
Then you can bootstrap with 'juju bootstrap lxd-remote2'

Then verify with:

juju clouds
Cloud        Regions  Default          Type        Description
aws               15  us-east-1        ec2         Amazon Web Services
aws-china          1  cn-north-1       ec2         Amazon China
aws-gov            1  us-gov-west-1    ec2         Amazon (USA Government)
azure             26  centralus        azure       Microsoft Azure
azure-china        2  chinaeast        azure       Microsoft Azure China
cloudsigma         5  hnl              cloudsigma  CloudSigma Cloud
google            13  us-east1         gce         Google Cloud Platform
joyent             6  eu-ams-1         joyent      Joyent Cloud
oracle             5  uscom-central-1  oracle      Oracle Cloud
rackspace          6  dfw              rackspace   Rackspace Cloud
localhost          1  localhost        lxd         LXD Container Hypervisor
lxd                1  default          lxd         LXD Container Hypervisor
lxd-remote         0                   lxd         LXD Container Hypervisor
lxd-remote2        1  default          lxd         LXD Container Hypervisor

Documentation changes

Allows you to set a default region when adding a cloud, so that when
calling juju clouds the default region is correctly set.

Bug reference

N/A

Update the pollster interaction so that we can have default naming
for object key names. To improve the UI/UX for adding a LXD cloud
interactively we want to state that there can be a region and that
it can be default, we do allow you to override this when adding a
cloud, but a sane default should suffice.
@SimonRichardson
Copy link
Member Author

@SimonRichardson
Copy link
Member Author

Copy link
Member

@manadart manadart left a comment

Choose a reason for hiding this comment

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

As discussed with @SimonRichardson

Interesting, that if you created different LXD clusters (and assuming the network set-up allowed containers to communicate), you could add them as regions to one LXD cloud.

This would be a way of getting around the requirement of homogeneous machines per LXD cluster - a vanilla cluster, a GPU cluster etc as regions of a single cloud.

Ping @mitechie with these musings.

@SimonRichardson
Copy link
Member Author

$$merge$$

@jujubot jujubot merged commit 2585aa9 into juju:develop Aug 10, 2018
Singular: "region",
Plural: "regions",
Default: "default",
PromptDefault: "use default",
Copy link
Member

Choose a reason for hiding this comment

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

since the Default is "default", having PromptDefault as "use default" had me confused. Whether "default" was literal or not. Suggest not setting a PromptDefault. Then the Default will be picked up for the prompt, e.g.:
Enter region name [default]:

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix the wording...

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #9057

@@ -65,6 +65,7 @@ type environProvider struct {
var cloudSchema = &jsonschema.Schema{
Type: []jsonschema.Type{jsonschema.ObjectType},
Required: []string{cloud.EndpointKey, cloud.AuthTypesKey},
Order: []string{cloud.EndpointKey, cloud.AuthTypesKey, cloud.RegionsKey},
// Order doesn't matter since there's only one thing to ask about. Add
Copy link
Member

Choose a reason for hiding this comment

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

comment no longer valid?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, I'll remove

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in #9057

if schema.Default != nil {
var def string
if schema.PromptDefault != nil {
def = fmt.Sprintf("%v", schema.PromptDefault)
Copy link
Member

Choose a reason for hiding this comment

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

any chance you want to add allowing for an env var here as with pr 9015? then we can uncomment provider/openstack/provider.go: 126 and remove the TODO. :-D

                    if schema.PromptDefault != nil {
                            def = fmt.Sprintf("%v", schema.PromptDefault)
                    }
                    var defFromEnvVar string
                    if len(schema.EnvVars) > 0 {
                            for _, envVar := range schema.EnvVars {
                                    value := os.Getenv(envVar)
                                    if value != "" {
                                            defFromEnvVar = value
                                            def = defFromEnvVar
                                            break
                                    }
                            }
                    }
                    if def == "" {
                            def = fmt.Sprintf("%v", schema.Default)
                    }

and a change at line 415 perhaps too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's do that in another PR

@hmlanigan
Copy link
Member

There is a small bug... You can now juju add-cloud for openstack and not provide a region, where this wasn't possible before.

@SimonRichardson
Copy link
Member Author

Wow, that's really strange... I'll fix asap Monday

@SimonRichardson
Copy link
Member Author

It's actually because the default is "", which seems strange as it should be nil imo for openstack, will have a think if openstack is wrong or we should fix this code instead

@SimonRichardson SimonRichardson deleted the default-schema-region branch August 13, 2018 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants