Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Create interactive add-cloud command #6498
Conversation
natefinch
changed the title from
WIP: Create interactive add-cloud command
to
Create interactive add-cloud command
Nov 1, 2016
|
Please re-target against develop :D |
natefinch
changed the base branch from
master
to
develop
Nov 1, 2016
|
Tips for code reviewers: Start with query.go
you can then look at query_test.go for the minimal changes required there. Bootstrap interactive is the only other file currently using QueryVerify, but it needed to be updated to use the new format. After that, take a look at pollster.go which holds all the code for repeatedly querying the user (holds the bufio.scanner internally), and also what we use to transform jsonschema into user queries. This is the big one. The basic idea is that it just wraps QueryVerify... for jsonschema, it iterates through the schema, and based on what the schema says, asks the user for information. Currently it is purpose-built for querying about providers, and doesn't support a ton of jsonschema that we don't need, for example, it doesn't support optional parameters, ebcause we don't have any yet. It'll get more complicated as we need to support a wider variety of jsonschema, but for now I just tried to do sanity checks and actively error out if you try to do something it doesn't understand. After that you can look at add.go which uses the pollster to make add-cloud interactive if you don't give And then you can look at the tests for the above in add_test.go The rest is just the cloud schema definitions for the clouds we want to support with add-cloud interactive (i.e. maas, manual, openstack, and vsphere), and some other minor bits and bobs that were necessary to effect the change. |
reedobrien
suggested changes
Nov 3, 2016
•
The rackspace fakeprovider is missing the CloudSchema method.
# github.com/juju/juju/provider/rackspace_test
provider/rackspace/provider_test.go:27: cannot use s.innerProvider (type *fakeProvider) as type environs.EnvironProvider in argument to rackspace.NewProvider:
*fakeProvider does not implement environs.EnvironProvider (missing CloudSchema method)
I'm pretty tired and I feel like I'm missing or misunderstanging some stuff. I'll look again with fresh eyes later but it'll need a second review anyhow due to size.
Only checked the 1st QA step at this point.
| @@ -70,15 +80,22 @@ func (c *addCloudCommand) SetFlags(f *gnuflag.FlagSet) { | ||
| } | ||
reedobrien
Nov 3, 2016
Contributor
A bunch of these exported Command interface impl methods have, no comment.
| + return addCloud(c.Cloud, newCloud) | ||
| +} | ||
| + | ||
| +func (c *addCloudCommand) runInteractive(ctxt *cmd.Context) error { |
reedobrien
Nov 3, 2016
Contributor
This is a pretty huge method. Is it feasible to break it up a little?
| + break | ||
| + } | ||
| + // we do this instead of returning the error message | ||
| + override, err2 := pollster.YN(err.Error()+", do you want to override that definition", false) |
reedobrien
Nov 3, 2016
Contributor
Would this overwrite an existing definition? override from here doesn't sound too destructive. If it is destructive it should probably be louder about it.
natefinch
Nov 3, 2016
Contributor
it's destructive if it's overriding one you created, but not destructive if it's overriding a default one (like EC2). Although, in the interactive case, we don't even support overriding a default one, so that point is kind of moot. So, yes, it's destructive. I can make the error message a little scarier.
| + ctxt.Infof("You may bootstrap with 'juju bootstrap %s'", name) | ||
| + } | ||
| + return err | ||
| +} |
reedobrien
Nov 3, 2016
Contributor
Should this be more like?
if err != nil {
return errors.Trace(err)
}
ctxt.Infof("Cloud %q successfully added", name)
ctxt.Infof("You may bootstrap with 'juju bootstrap %s'", name)
return nil
or something
| +type errWriter struct { | ||
| + w *ansiterm.Writer | ||
| +} | ||
| + |
| + return nil | ||
| +} | ||
| + | ||
| +func querySchema(schema *jsonschema.Schema, pollster *interact.Pollster) error { |
natefinch
Nov 3, 2016
Contributor
oops, nobody. this is just extraneous (I refactored it out of use)
| @@ -90,7 +92,7 @@ func (s *addSuite) TestAddBadCloudName(c *gc.C) { | ||
| func (s *addSuite) TestAddExisting(c *gc.C) { | ||
| sourceFile := s.createTestCloudData(c) | ||
| _, err := testing.RunCommand(c, cloud.NewAddCloudCommand(), "homestack", sourceFile) | ||
| - c.Assert(err, gc.ErrorMatches, `\"homestack\" already exists; use --replace to replace this existing cloud`) | ||
| + c.Assert(err, gc.ErrorMatches, `"homestack" already exists; use --replace to override this definition`) |
reedobrien
Nov 3, 2016
Contributor
Override makes me think you can remove this definition and get back the existing cloud. If so good. If not I think it should be more explicit that we're deleting the original.
I think override might be appropriate in the context of a builtin/public cloud, but if it is one that is only defined locally and we are overwriting it -- which, IIUC we are here -- we should be explicit about destroying the existing entry.
| query := fmt.Sprintf("Select a cloud [%s]: ", defCloud) | ||
| - cloud, err := interact.QueryVerify([]byte(query), scanner, w, verify) | ||
| + cloud, err := interact.QueryVerify(query, scanner, w, w, verify) |
reedobrien
Nov 3, 2016
Contributor
Seems odd that this call takes the same writer twice. I looked and understand why, but a comment would be nice for future readers. Here and below, dunno. Or maybe an unexported method with the signature fixed with an explanation.
natefinch
Nov 4, 2016
Contributor
so, I'm planning to migrate this code very soon to use pollster instead, so I think I'd like to avoid putting a bunch of comments here that will be obviated in my next PR.
| + Default string | ||
| +} | ||
| + | ||
| +// MultiList contains the information necessary to ask the user to select form a list |
| +var selectTmpl = template.Must(template.New("").Parse(` | ||
| +Select {{.Singular}}{{if .Default}} [{{.Default}}]{{end}}: `[1:])) | ||
| + | ||
| +// Select queries the user to select from the give list of options. |
| + return p.SelectVerify(l, VerifyOptions(l.Singular, l.Options, l.Default != "")) | ||
| +} | ||
| + | ||
| +// SelectVerify queries the user to select from the give list of options, using |
reedobrien
Nov 3, 2016
Contributor
s/give/given
Not sure I understand what the bit after the comment means -- It reads like 'select from the list of options using the custom verifier listed.'
| + map[string]interface{}{"join": strings.Join}).Parse(` | ||
| +Select one or more {{.Plural}} separated by commas{{if .Default}} [{{join .Default ", "}}]{{end}}: `[1:])) | ||
| + | ||
| +// MultiSelect queries the user to select one more answers from the give list of |
reedobrien
Nov 3, 2016
Contributor
s/give/given
I assume there's a check somewhere for extraneous commas, since there's a note that there shouldn't be any.
natefinch
Nov 4, 2016
Contributor
Not sure about "extra", but when you enter text, it is split on commas. There's no actual sanity check that the options themselves don't contain commas (which would make them unusable)... I'll add that.
| +// error, and the other return values are ignored. If ok is true, the value is | ||
| +// acceptable, and that value will be returned by the calling function. If ok | ||
| +// is false, the user will be asked to enter a new value for query. If ok is | ||
| +// false. if errmsg is not empty, it will be printed out as an error to te the |
reedobrien
Nov 3, 2016
Contributor
If ok is false. if errmsg is not empty,
wot?
I think I get it. Seems really complicated. Error also seems overloaded. Maybe 'hint' or something would convey better. It seems that it could even return simply string, error and if the hint != "" output the hint else it's fine move on.
natefinch
Nov 3, 2016
Contributor
So.... error is an error - something went badly wrong, we should bail out.
ok is whether or not the string passes verification.
the message is what is shown if the message fails verification.
The reason we can't just have a string and check for empty is that there are times when we want to fail verification but not print anything out. For now, that basically only happens if the input is an empty string and there's no default. Instead of printing out "" is not a valid foo, we just redisplay the query.
| +// YN queries the user with a yes no question q (which should not include a | ||
| +// question mark at the end). It uses def as the default answer. | ||
| +func (p *Pollster) YN(q string, def bool) (bool, error) { | ||
| + defstring := "(y/N)" |
reedobrien
Nov 3, 2016
Contributor
defString ?
or even defaultStr or something. I was reading it as define string at first, even after reading the doc comment:/
natefinch
Nov 3, 2016
Contributor
yeah, that's better. There's times when I really wish default was not a keyword :)
| + if err != nil { | ||
| + return false, errors.Trace(err) | ||
| + } | ||
| + return yesNoConvert(a, def) |
reedobrien
Nov 3, 2016
Contributor
I don't understand at a glance why we run yesNoConvert again here. if it verified in the VerifyFunc and there was no error why not return true, nil?
Sadly, I didn't take your advice and start with query.go .... so I might be missing something.
natefinch
Nov 3, 2016
Contributor
queryVerify returns a string. we need to return a boolean. So, it's either "y" (true) or "n" (false).
| + return nil | ||
| +} | ||
| + | ||
| +func (p *Pollster) queryOneSchema(schema *jsonschema.Schema) (interface{}, error) { |
reedobrien
Nov 3, 2016
Contributor
This is pretty long. Is it possible to break it into a couple functions? It looks like it will just get longer if supporting more formats. So maybe wait to refactor, but maybe less daunting while it is still under 50 lines.
| +} | ||
| + | ||
| +func (p *Pollster) selectOne(schema *jsonschema.Schema) (interface{}, error) { | ||
| + options := make([]string, len(schema.Enum)) |
| + return false | ||
| +} | ||
| + | ||
| +func convArray(vals []string, schema *jsonschema.Schema) (interface{}, error) { |
| + return s, nil | ||
| + case jsonschema.BooleanType: | ||
| + switch strings.ToLower(s) { | ||
| + case "y", "yes", "true", "t": |
reedobrien
Nov 3, 2016
Contributor
Nobody would ever type a "1" or "0" here would they? Nah, not worth handling. Right?
natefinch
Nov 3, 2016
•
Edited 1 time
-
natefinch
Nov 4, 2016
Contributor
-
Nov 4, 2016
I actually intentionally did not support 0 or 1, because I don't want anyone writing a prompt that requires someone to know that 1 is true and 0 is false. I almost didn't support true and false, but those are at least english words with specific meanings.
| return answer, nil | ||
| } | ||
| + | ||
| + if done { | ||
| + // can't query any more, nothing we can do. |
reedobrien
Nov 3, 2016
Contributor
What if there's a msg here? Is it impossible? I'm getting tired. I think I'm missing things.
natefinch
Nov 3, 2016
Contributor
There might be a message.... I guess it's possible we got io.EOF from stdin but still can print to stdout. However, it's pretty likely that everything is going to have to error out at this point. It should be one of those "it'll never happen" things, but we can try to handle it gracefully anyway.
| @@ -21,7 +22,7 @@ type EnvironProvider interface { | ||
| config.Validator | ||
| ProviderCredentials | ||
| - // TODO(wallyworld) - embed config.ConfigSchemaSource and make all providers implement it | ||
| + CloudSchema() *jsonschema.Schema |
| @@ -97,6 +98,10 @@ func (prov *azureEnvironProvider) Open(args environs.OpenParams) (environs.Envir | ||
| return environ, nil | ||
| } | ||
| +func (p azureEnvironProvider) CloudSchema() *jsonschema.Schema { |
| @@ -80,6 +81,9 @@ func (environProvider) Open(args environs.OpenParams) (environs.Environ, error) | ||
| return env, nil | ||
| } | ||
| +func (p environProvider) CloudSchema() *jsonschema.Schema { |
| @@ -612,6 +613,9 @@ func (p *environProvider) Open(args environs.OpenParams) (environs.Environ, erro | ||
| } | ||
| return env, nil | ||
| } | ||
| +func (p environProvider) CloudSchema() *jsonschema.Schema { |
| + }, | ||
| +} | ||
| + | ||
| +// CloudSchema returns the schema for verifying cloud |
voidspace
commented
Nov 4, 2016
|
Works fine for me, looking through the code. My only gripe is that I have to add credentials separately, adding a cloud isn't enough to use it. That's by design though, so only a gripe :-) |
|
!!go!! |
reedobrien
approved these changes
Nov 4, 2016
LGTM. Should get a second if you don't already have it.
| @@ -97,6 +98,12 @@ func (prov *azureEnvironProvider) Open(args environs.OpenParams) (environs.Envir | ||
| return environ, nil | ||
| } | ||
| +// CloudSchema returns the schema used to validate input for add-cloud. Since | ||
| +// this provider does not support custom clouds, this always returns nil. | ||
| +func (p azureEnvironProvider) CloudSchema() *jsonschema.Schema { |
voidspace
Nov 7, 2016
It doesn't look to me like any of these provider changes (CloudSchema implementations) are tested?
voidspace
reviewed
Nov 7, 2016
Other than the lack of tests for the CloudSchema implementations, it looks good to me.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
natefinch commentedOct 25, 2016
•
Edited 1 time
-
natefinch
Nov 1, 2016
This PR adds an interactive way to add a cloud via answering prompts
rather than having to specify a premade yaml file.
It also includes changes to the interact package to support
standardized ways of querying the user for information, which
can be reused for other interactive commands.
QA:
run
juju add-cloudShould show options to create clouds of types maas, manual, openstack, and vsphere.
Should prompt for all required information to add the cloud, which should be added to your clouds.yaml.
You should then be able to use that cloud definition with juju bootstrap.