asset/*: allow prompts to be read from environment #320
Conversation
This adds support for reading user prompts from the environment. This
will allow CI to test the installer without needing `expect` or a
similar utility. It can be used like the following:
OPENSHIFT_INSTALL_CLUSTER_NAME=crawford
OPENSHIFT_INSTALL_PLATFORM=libvirt
openshift-install cluster
openshift-ci-robot
added
size/L
| Message: "Platform", | ||
| Options: validPlatforms, | ||
| }, &platform, nil) | ||
| func (a *Platform) queryUserForPlatform() (string, error) { |
There was a problem hiding this comment.
maybe rename this to queryUserOrGetEnvForPlatform
There was a problem hiding this comment.
maybe rename this to
queryUserOrGetEnvForPlatform
I'm fine with the old name. Environment variables are just another channel for the user to answer these queries.
There was a problem hiding this comment.
So you'd rather double-check each function where it gets the input from instead of having the information readily available in the name?
There was a problem hiding this comment.
So you'd rather double-check each function where it gets the input from...
Yeah. The caller just needs to care that the input is gotten. The source location is an implementation detail.
| Default: "qemu+tcp://192.168.122.1/system", | ||
| }, &uri, nil) | ||
| prompt := asset.UserProvided{ | ||
| Prompt: &survey.Input{ |
There was a problem hiding this comment.
Did you forget to use &survey.Select here?
There was a problem hiding this comment.
No. This is a free-form field.
There was a problem hiding this comment.
Do I understand correctly that due to https://github.com/openshift/installer/pull/320/files#diff-042a71691f31141034ea54bae5b36b06R26 this is settable via environment variable and will only ask interactively if it's not present?
|
|
||
| // Generate generates the SSH public key asset. | ||
| func (a *sshPublicKey) Generate(map[asset.Asset]*asset.State) (state *asset.State, err error) { | ||
| if value, ok := os.LookupEnv("OPENSHIFT_INSTALL_SSH_PUB_KEY"); ok { |
There was a problem hiding this comment.
I'd consider to also ask for the SSH key if not provided, or are there use-cases where one wouldn't want one set?
There was a problem hiding this comment.
I'd consider to also ask for the SSH key if not provided, or are there use-cases where one wouldn't want one set?
I'm fine allowing folks to not set one (e.g. if they trust us to set up a working cluster for them so they won't have to poke around ;). So I think we want to allow empty OPENSHIFT_INSTALL_SSH_PUB_KEY to mean "I don't want to set this, don't ask me". Alternatively, we could have a --batch flag or some such to mean "anything I haven't passed to you (via environment variables or otherwise) is not coming, so don't ask."
There was a problem hiding this comment.
Setting OPENSHIFT_INSTALL_SSH_PUB_KEY to an empty string effectively boots without any keys. There is an empty key, but that's okay.
There was a problem hiding this comment.
Alternatively, we could have a
--batchflag or some such to mean "anything I haven't passed to you (via environment variables or otherwise) is not coming, so don't ask."
That sounds useful. If someone goes through it the first time interactively they might forget or not be aware of this setting and later have to restart when they find themselves unable to login.
There was a problem hiding this comment.
If someone goes through it the first time interactively they might forget or not be aware of this setting and later have to restart when they find themselves unable to login.
The long-term plan is to have one of the operators (the machine-config operator?) managing authorized SSH keys over the life of the cluster. So you should (eventually) be able to recover even if you launch a cluster without this.
|
/hold Please feel free to release the old after considering the following UX features |
openshift-ci-robot
added
the
do-not-merge/hold
|
I'm holding off on documentation for now. I've got a large rewrite in the works. As for the |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
@wking Did you have any final comments? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: crawford, steveeJ, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
5 participants
This adds support for reading user prompts from the environment. This
will allow CI to test the installer without needing
expector asimilar utility. It can be used like the following: