Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Print available bootstrap config options #7449
Conversation
| @@ -21,7 +25,8 @@ type showCloudCommand struct { | ||
| var showCloudDoc = ` | ||
| Provided information includes 'defined' (public, built-in), 'type', | ||
| -'auth-type', 'regions', and 'endpoints'. | ||
| +'auth-type', 'regions', 'endpoints', and cloud specific bootstrap |
| + BootstrapConfig map[string]interface{} `yaml:"bootstrap-config,omitempty" json:"bootstrap-config,omitempty"` | ||
| + ModelConfig map[string]interface{} `yaml:"model-config,omitempty" json:"model-config,omitempty"` | ||
| + RegionConfig jujucloud.RegionConfig `yaml:"region-config,omitempty" json:"region-config,omitempty"` | ||
| + ProviderConfig map[string]interface{} `yaml:"supported-cloud-config,omitempty" json:"supported-cloud-config,omitempty"` |
wallyworld
Jun 13, 2017
Owner
Or just "cloud-config".
That then fits with "model-config", "region-config" etc.
| + } | ||
| + // Not all providers have implemented the ProviderSchema interface, | ||
| + // e.g. joyent, double check before using. | ||
| + if m, ok := interface{}(provider).(environs.ProviderSchema); ok { |
wallyworld
Jun 13, 2017
Owner
Can we invert this logic to return early if !ok
Also, we don't need the "interface{}"...
m, ok := provider.(environs.ProviderSchema)
| + // specific config option names, but no | ||
| + // description etc. | ||
| + for attr := range ps.ConfigSchema() { | ||
| + if !providerSchema[attr].Secret { |
wallyworld
Jun 13, 2017
Owner
To avoid excessive indenting, we can just
if providerSchema[attr].Secret {
continue
}
| @@ -55,24 +85,26 @@ clouds: | ||
| endpoint: http://london/1.0 | ||
| config: | ||
| bootstrap-timeout: 1800 |
wallyworld
Jun 13, 2017
Owner
bootstrap-timeout is a bootstrap only config attr so it should be under bootstrap-config right?
Also, we need tests where the cloud.yaml file defines generic model config in addtion to bootstrap config and cloud specific config
hmlanigan
Jun 14, 2017
Member
Line 87 is the config.yaml file used for show-cloud, so it should be config. At line 107, bootstrap-timeout is shown in as part of bootstrap-config.
What do we expect the region-config output to look like with these changes?
region-config:
london:
bootstrap-config:
bootstrap-timeout: 1800
model-config:
use-floating-ip: true
wallyworld
Jun 14, 2017
Owner
Oh, sorry, I misread the diff. Yes, this is clouds.yaml not the output.
hmlanigan
Jun 14, 2017
Member
The last change is made, add bootstrap-config and model-config to region-config output. Updated TestShowWithRegionConfig() to test all of it together.
wallyworld
Jun 15, 2017
Owner
I think it's fine just to have all the region config lumped together. There will only be a few overrides in practice. We can iterate it that becomes an issue.
| @@ -95,8 +127,8 @@ clouds: | ||
| ctx, err := cmdtesting.RunCommand(c, cloud.NewShowCloudCommand(), "homestack") | ||
| c.Assert(err, jc.ErrorIsNil) | ||
| out := cmdtesting.Stdout(ctx) | ||
| - c.Assert(out, gc.Equals, ` | ||
| -defined: local | ||
| + //c.Assert(out, gc.Equals, ` |
| @@ -178,6 +181,7 @@ func (c *bootstrapCommand) SetFlags(f *gnuflag.FlagSet) { | ||
| f.StringVar(&c.AgentVersionParam, "agent-version", "", "Version of tools to use for Juju agents") | ||
| f.StringVar(&c.CredentialName, "credential", "", "Credentials to use when bootstrapping") | ||
| f.Var(&c.config, "config", "Specify a controller configuration file, or one or more configuration\n options\n (--config config.yaml [--config key=value ...])") | ||
| + f.StringVar(&c.showConfigOptionsForCloud, "show-config-options", "", "Print available config options for the specified cloud type") |
| -// a juju in that environment if none already exists. If there is as yet no environments.yaml file, | ||
| -// the user is informed how to create one. | ||
| +// a juju in that environment if none already exists. If there is as yet no | ||
| +// environments.yaml file, the user is informed how to create one. |
| "github.com/juju/juju/jujuclient" | ||
| + | ||
| + "gopkg.in/juju/environschema.v1" |
| + } else { | ||
| + cloudType = showCloudConfigInput | ||
| + } | ||
| + provider, err := environs.Provider(cloudType) |
wallyworld
Jun 13, 2017
Owner
This logic to get the provider config should be factored out to a shared function used by both show-cloud and bootstrap
|
This looks like a great improvement. What's holding up getting it finished? Lack of bandwidth? (Just making sure it hasn't been forgotten) |
|
It's not forgotten, travel for work got in the way. Working on resolving the comments, one left to address. |
|
This seems like a very strange fit of commands. A flag on a command that makes it not function but instead shows information? Using show-xxxx in a flag vs a command? This strikes me as an odd UX mix. This reads like it's help content vs actually pulling any information from the model as you can pass other providers through. I'd much rather we have something in help that points to something like juju help cloud-config. Is there any other commands that behave in this way that we're attempting to be consistent with? |
wallyworld
approved these changes
Jun 15, 2017
I think we need to alter the final show-clouds output as per comment. Let's discuss.
| + | ||
| +// groupRegionConfigValues returns config variables grouped by bootstrap-config | ||
| +// and model-config for each region with config variables. | ||
| +func groupRegionConfigValues(regionConfigs jujucloud.RegionConfig) map[string]interface{} { |
| @@ -55,24 +85,26 @@ clouds: | ||
| endpoint: http://london/1.0 | ||
| config: | ||
| bootstrap-timeout: 1800 |
wallyworld
Jun 13, 2017
Owner
bootstrap-timeout is a bootstrap only config attr so it should be under bootstrap-config right?
Also, we need tests where the cloud.yaml file defines generic model config in addtion to bootstrap config and cloud specific config
hmlanigan
Jun 14, 2017
Member
Line 87 is the config.yaml file used for show-cloud, so it should be config. At line 107, bootstrap-timeout is shown in as part of bootstrap-config.
What do we expect the region-config output to look like with these changes?
region-config:
london:
bootstrap-config:
bootstrap-timeout: 1800
model-config:
use-floating-ip: true
wallyworld
Jun 14, 2017
Owner
Oh, sorry, I misread the diff. Yes, this is clouds.yaml not the output.
hmlanigan
Jun 14, 2017
Member
The last change is made, add bootstrap-config and model-config to region-config output. Updated TestShowWithRegionConfig() to test all of it together.
wallyworld
Jun 15, 2017
Owner
I think it's fine just to have all the region config lumped together. There will only be a few overrides in practice. We can iterate it that becomes an issue.
| + bootstrap-timeout: 1800 | ||
| + model-config: | ||
| + use-floating-ip: true | ||
| +`, openstackProviderConfig}, "")) |
wallyworld
Jun 15, 2017
Owner
I'm starting to think that this block of provider specific config should be printed separately as it is not "what's defined in clouds.yaml" (which is what show-cloud nominally prints), but metadata. So something like below (ignore bad formatting). Sorry for churn, it's taken a few iterations to get a feel for it.
`$ juju show-cloud myopenstack
defined: local
type: openstack
description: Openstack Cloud
auth-types: [userpass, access-key]
endpoint: http://homestack
regions:
london:
endpoint: http://london/1.0
config:
max-logs-age: 2m
max-logs-size: 1M
region-config:
london:
max-logs-age: 4m
The available config options specific to openstack clouds are:
external-network:
type: string
description: The network label or UUID to create floating IP addresses on when
multiple external networks exist.
network:
type: string
description: The network label or UUID to bring machines up on when multiple networks
exist.
use-default-secgroup:
type: bool
description: Whether new machine instances should have the "default" Openstack
security group assigned in addition to juju defined security groups.
use-floating-ip:
type: bool
description: Whether a floating IP address is required to give the nodes a public
IP address. Some installations assign public IP addresses by default without
requiring a floating IP address.`
| + } | ||
| + // providerSchema has all config options, including their descriptions | ||
| + // and types. | ||
| + if providerSchema, err := common.CloudSchemaByType(cloudType); err == nil { |
wallyworld
Jun 15, 2017
Owner
To avoid excessive indenting, we should make this call outside of the if statement and do the error checks, and then proceed. Same below.
|
Content and approach changed per conversation with @wallyworld and @mitechie. This PR will cover a new juju show-cloud flag: --include-config and related updates to help output and tests. Additional related updates to help and docs will be done in other PRs. |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
hmlanigan commentedJun 6, 2017
•
Edited 1 time
-
hmlanigan
Jun 14, 2017
Description of change
This change is to help improve juju usability by providing a method for users to
obtain possible config options for bootstrap before bootstrapping.
The output of juju show-cloud --include-config will now include a description of config
options unique to the specific cloud and differentiate bootstrap config options from ones
for a model.
The intent is to ask for user feedback on the new output and adjust accordingly.
QA steps
Run:
Documentation changes
Unknown.
Bug reference
No