-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
asset/*: re-implement user prompts #287
Conversation
b00e40e erroneously added a bunch of unneeded files.
`glide update` hadn't been run after the Tectonic Node Controller was removed.
This library provides nice terminal prompting functionality that the installer will use to read input from the user.
This supersedes #212. |
/hold I still want to fix a couple things. |
Help: "The AWS region to be used for installation.", | ||
Default: "us-east-1 (N. Virginia)", | ||
Options: []string{ | ||
"us-east-2 (Ohio)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you work this up by hand? I'd rather auto-generate it. You can get close with:
$ aws ec2 describe-regions --query Regions --output json | jq '[.[] | .RegionName] | sort'
[
"ap-northeast-1",
"ap-northeast-2",
"ap-south-1",
"ap-southeast-1",
"ap-southeast-2",
"ca-central-1",
"eu-central-1",
"eu-west-1",
"eu-west-2",
"eu-west-3",
"sa-east-1",
"us-east-1",
"us-east-2",
"us-west-1",
"us-west-2"
]
But I can't find a way to get the human names ("Ohio" here). There's more here (including "how do you get the human name?"). Amazon provides this.
The CLI query also doesn't list GovCloud regions (presumably because I don't have access to any of those). And the AWS page talks about plans for additional future regions. So maybe just throw up our hands and don't list options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied the list from AWS's documentation.
My thinking with the regions was that if the user couldn't find their region, they could just modify install-config.yaml after the fact. This list won't include GovCloud or AWS China.
pkg/asset/installconfig/ssh.go
Outdated
survey.AskOne(&survey.Select{ | ||
Message: "SSH Public Key", | ||
Help: "The SSH key used to access all nodes within the cluster. This is optional.", | ||
Options: append([]string{none}, paths...), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you also want to continue to support "paste in your SSH pubkey" (vs. just reading it from a file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just tell the user to edit their install config if they want to do that. Remember, this interactive UI is for the basic use case - nothing fancy.
path := paths[i-1] | ||
input = pubKeys[path] | ||
} else { | ||
err = validate.OpenSSHPublicKey(input) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to keep this validation for when a user pastes in a pubkey directly.
This uses a library instead of a custom implementation. This has the advantage of providing defaults, help messages, and a nicer interface.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, crawford The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
home := os.Getenv("HOME") | ||
if home != "" { | ||
paths, err = filepath.Glob(filepath.Join(home, ".ssh", "*.pub")) | ||
paths, err := filepath.Glob(filepath.Join(home, ".ssh", "*.pub")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I liked the old way better, instead of using an if
-scoped path
here, throwing it away, and then (re)building a function-scoped path
later. Not worth blocking the merge though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All I did was move the construction of paths outside of this block. The pubkeys map is really the output of this block.
{Data: []byte(input)}, | ||
}, | ||
Contents: []asset.Content{{ | ||
Data: pubKeys[path], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're still not supporting "paste in your SSH pubkey". Is that going to be another "edit your install-config YAML afterwards if you need it"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Copying from above because I'm lazy:
We can just tell the user to edit their install config if they want to do that. Remember, this interactive UI is for the basic use case - nothing fancy.
/retest |
/hold cancel |
This uses a library instead of a custom implementation. This has the
advantage of providing defaults, help messages, and a nicer interface.