Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Bootstrap config: inherited overwrites defaults. #6496
Conversation
dimitern
approved these changes
Oct 25, 2016
Looks good, but since it's >500 lines, please ask for a second review.
| + | ||
| +// Get the credentials and region name. | ||
| +func (c *bootstrapCommand) getCredentialsAndRegionName( | ||
| + ctx *cmd.Context, cloud *jujucloud.Cloud) ( |
dooferlad
Oct 25, 2016
Contributor
That would need an extra comma:
func (c *bootstrapCommand) credentialsAndRegionName(
ctx *cmd.Context, cloud *jujucloud.Cloud,
) (*jujucloud.Credential, string, string, string, error) {Personally I think that looks worse. How about leave as is and then I propose a followup where I return / fill in a struct?
| +} | ||
| + | ||
| +func (c *bootstrapCommand) getBootstrapConfigs( | ||
| + ctx *cmd.Context, cloud *jujucloud.Cloud, provider environs.EnvironProvider) ( |
| + | ||
| +func (c *bootstrapCommand) getBootstrapConfigs( | ||
| + ctx *cmd.Context, cloud *jujucloud.Cloud, provider environs.EnvironProvider) ( | ||
| + map[string]interface{}, controller.Config, bootstrap.Config, map[string]interface{}, map[string]interface{}, error) { |
dimitern
Oct 25, 2016
Contributor
I'd even suggest using a formatting like this:
func (c *bootstrapCommand) getBootstrapConfigs(
ctx *cmd.Context,
cloud *jujucloud.Cloud,
provider environs.EnvironProvider,
) (
map[string]interface{}, // bootstrapModelConfig
controller.Config,
bootstrap.Config,
map[string]interface{}, // inheritedControllerAttrs
map[string]interface{}, // userConfigAttrs
error,
) {
...
}| + c *gc.C, | ||
| + bootstrapCmd bootstrapCommand, | ||
| + key string, | ||
| + ctx *cmd.Context, cloud *cloud.Cloud, provider environs.EnvironProvider, |
dimitern
Oct 25, 2016
Contributor
I'd suggest:
func checkConfigs(
c *gc.C,
bootstrapCmd bootstrapCommand,
key string,
ctx *cmd.Context,
cloud *cloud.Cloud,
provider environs.EnvironProvider,
expect map[string]map[string]interface{},
) {
...
}
kat-co
Oct 25, 2016
Contributor
+1. Prefer long functions to be made to look like structs. Inconsistent numbers of parameters on a line is frowned upon.
| + | ||
| +// checkConfigEntryMatches tests that a keys existence and indexed value in configMap | ||
| +// matches those in expect[name]. | ||
| +func checkConfigEntryMatches(c *gc.C, configMap map[string]interface{}, key, name string, expect map[string]map[string]interface{}) { |
dimitern
Oct 25, 2016
Contributor
Ditto:
func checkConfigEntryMatches(
c *gc.C,
configMap map[string]interface{},
key, name string,
expect map[string]map[string]interface{},
) {
...
}| +} | ||
| + | ||
| +func (s *BootstrapSuite) TestBootstrapAttributesInheritedOverDefaults(c *gc.C) { | ||
| + /* Test that defaults are overwritten by inherited attributes by setting |
| +} | ||
| + | ||
| +func (s *BootstrapSuite) TestBootstrapAttributesCLIOverDefaults(c *gc.C) { | ||
| + /* Test that defaults are overwritten by CLI passed attributes by setting |
| +} | ||
| + | ||
| +func (s *BootstrapSuite) TestBootstrapAttributesCLIOverInherited(c *gc.C) { | ||
| + /* Test that defaults are overwritten by CLI passed attributes by setting |
|
Tested with no cli option, but with clouds.yaml option, saw success: |
perrito666
suggested changes
Oct 25, 2016
Hey I made a few comments that at first might look like aesthetic only but I believe that are a potential for introduction of hard to track errors especially the passing of structs instead of many string/other parameters to methods). Regarding the use of get in many methods it is anti-idiomatic in go so I believe its rather important for code quality, cheers.
| - return errors.Errorf("--bootstrap-image must be used with --bootstrap-constraints, specifying architecture") | ||
| - } | ||
| + | ||
| + // Start by checking for usage errors, equests for information |
| + bootstrapConfig, | ||
| + inheritedControllerAttrs, | ||
| + userConfigAttrs, | ||
| + err := c.getBootstrapConfigs(ctx, cloud, provider) |
perrito666
Oct 25, 2016
Contributor
This seems a bit awkward It would be nicer to read if c.getBootstrapConfigs would return a struct containing all of these things and even more go idiomatic if it was called c.bootstrapConfigs
dooferlad
Oct 25, 2016
Contributor
I was trying to leave the surrounding code unmodified as much as possible to try and reduce the diff. It would perhaps be a reasonable cleanup follow-up to change this because it would be an obvious substitution of thingConfig with config.Thing. What do you think?
Will change the function name now.
perrito666
Oct 25, 2016
Contributor
I agree with this as a follow up (as long as the follow up happens ;) )
| +} | ||
| + | ||
| +// Get the credentials and region name. | ||
| +func (c *bootstrapCommand) getCredentialsAndRegionName( |
perrito666
Oct 25, 2016
Contributor
please make this receive a struct, so many string positionals is a potential mistake from a caller.
dooferlad
Oct 25, 2016
Contributor
As with above, this returns lots of variables to keep the diff clean. Another one for a followup?
| + return credential, credentialName, detectedCredentialName, regionName, nil | ||
| +} | ||
| + | ||
| +func (c *bootstrapCommand) getBootstrapConfigs( |
| + if err := common.FinalizeAuthorizedKeys(ctx, bootstrapModelConfig); err != nil { | ||
| + return nil, nil, bootstrap.Config{}, nil, nil, errors.Annotate(err, "finalizing authorized-keys") | ||
| + } | ||
| + logger.Debugf("preparing controller with config: %v", bootstrapModelConfig) |
kat-co
reviewed
Oct 25, 2016
•
I'm unsure how far you want to take the refactoring (I applaud the effort and think we should take the opportunity). I've left some comments I hope help.
I did not leave comments where there were formatting issues because I couldn't easily tell what was existing code and what was your code. In case it's your code: when function calls/declarations begin to get long, it's nice (but not required) to make it look like a struct, e.g.:
func foo(
fooParam interface{},
barParam interface{},
bazParam interface{},
) (
fooRetVal interface{},
barRetVal interface{},
) {
/*...*/
}Other than the existing state of the code, it LGTM; however, I would really like to get rid of the deferred closures.
| + | ||
| + // Run interactive bootstrap if needed/asked for | ||
| + if c.interactive { | ||
| + if err := c.runInteractive(ctx); err != nil { |
kat-co
Oct 25, 2016
Contributor
It is a bit of a smell that runInteractive doesn't return any information; i.e. it's modifying state. It would be more clear if it were to return the necessary information.
I don't think this is a blocking issue.
| + region, err := getRegion(cloud, c.Cloud, regionName) | ||
| + if err != nil { | ||
| + fmt.Fprintf(ctx.GetStderr(), | ||
| + "%s\n\nSpecify an alternative region, or try %q.\n", |
kat-co
Oct 25, 2016
Contributor
I like to use errors.Annotate for cases like this. It keeps the formatting of the error message uniform with other errors.
wallyworld
Oct 26, 2016
Owner
This is a user prompt though so the output that's needed is different to what annotate would print.
| + return errors.Annotate(err, "error reading current controller") | ||
| + } | ||
| + | ||
| + defer func() { |
kat-co
Oct 25, 2016
Contributor
Having this much in a deferred closure is a huge smell that this method is (still) doing too much. Can we please handle the error paths inline? This will force us to decompose this further until we're closer to the appropriate level.
| + ) | ||
| + | ||
| + // If we error out for any reason, clean up the environment. | ||
| + defer func() { |
kat-co
Oct 25, 2016
Contributor
This deferred closure in particular is extremely difficult to understand (I remember running into this when I first started). We should definitely pursue inlining this since it should only be necessary after the environment has begun bootstrapping.
I believe the reason we've written it this way is to avoid having to replicate the call to destroyEnvironment (there are three error-cases which would have to be handled). I think this is a hint that this function is (still) doing too much. We should have a function that does what it can to prepare an environment maybeBootstrapEnvironment which returns the necessary information to tear an environment down, and an error. Then the error should be checked, and if the other variables are populated. Something along the lines of:
environ, err := maybeBootstrapEnvironment(/*...*/)
if err != nil {
if environ != nil && c.KeepBrokenEnvironment == false {
environ.Teardown()
}
}This is so much clearer than a deferred closure with multiple conditionals.
| + }() | ||
| + | ||
| + // Block interruption during bootstrap. Providers may also | ||
| + // register for interrupt notification so they can exit early. |
kat-co
Oct 25, 2016
Contributor
This entire behavior should be stuffed in a function.
As an aside, I've always found this behavior to be odd. I usually just switch to a different terminal and kill the process. Do you happen to know why we don't allow ^c? Once this is in a function (blockSignals()?), the comment of the what this function is doing can be replaced with the why.
| + c *gc.C, | ||
| + bootstrapCmd bootstrapCommand, | ||
| + key string, | ||
| + ctx *cmd.Context, cloud *cloud.Cloud, provider environs.EnvironProvider, |
dimitern
Oct 25, 2016
Contributor
I'd suggest:
func checkConfigs(
c *gc.C,
bootstrapCmd bootstrapCommand,
key string,
ctx *cmd.Context,
cloud *cloud.Cloud,
provider environs.EnvironProvider,
expect map[string]map[string]interface{},
) {
...
}
kat-co
Oct 25, 2016
Contributor
+1. Prefer long functions to be made to look like structs. Inconsistent numbers of parameters on a line is frowned upon.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
jujubot
merged commit bebd627
into
juju:develop
Oct 26, 2016
1 check failed
|
Arg, flipping browser caching! I didn't see your comments @kat-co until I had hit merge. I am happy to do more refactoring when I have handled my next card. I have already done some test cleanups but I think progress on http://pad.lv/1633154 will keep @mitechie happy. |
|
Not a problem, @dooferlad. I've been bitten by that as well. Happy to see any of my suggestions picked up going forward. |
dooferlad commentedOct 25, 2016
•
Edited 1 time
-
dooferlad
Oct 25, 2016
Fixes an error in the bootstrap logic where a default from a cloud would overwrite values read from the users cloud config.
The bootstrap run logic has been split into several functions to aid readability and testing. The logical changes are actually very small and are contained within getBootstrapConfigs, which now builds up configuration by first combining all sources in order of lowest to highest precedence and then splitting the configuration out into bootstrap config, controller config and bootstrap model config. The old logic sort of did this, but it was muddled and had some precedence problems that were causing the fixed bugs.
Fixes http://pad.lv/1614239 and http://pad.lv/1619808
CI Steps
Bootstrap an OpenStack with config: use-floating-ip: true and check that it is honored. This needs to be set in clouds.yaml, not on the CLI to see the bug fix.
Notes to reviewers
The github diff viewer isn't being helpful, so open bootstrap.go and look at Run (line 314+) that used to be insanely big and make sure that it reads well. The only logical change is in getBootstrapConfigs (line 729+) where the old code was wrong. The other changes in bootstrap.go are just code being moved into functions and, otherwise, being left unaltered.