Skip to content

asset/*: allow prompts to be read from environment #320

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 1 commit into from
Sep 25, 2018
Merged

asset/*: allow prompts to be read from environment #320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 64 additions & 41 deletions pkg/asset/installconfig/platform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,11 @@ func (a *Platform) Dependencies() []asset.Asset {

// Generate queries for input from the user.
func (a *Platform) Generate(map[asset.Asset]*asset.State) (*asset.State, error) {
platform := a.queryUserForPlatform()
platform, err := a.queryUserForPlatform()
if err != nil {
return nil, err
}

switch platform {
case AWSPlatformType:
return a.awsPlatform()
Expand All @@ -57,61 +61,80 @@ func (a *Platform) Name() string {
return "Platform"
}

func (a *Platform) queryUserForPlatform() string {
var platform string
survey.AskOne(&survey.Select{
Message: "Platform",
Options: validPlatforms,
}, &platform, nil)
func (a *Platform) queryUserForPlatform() (string, error) {
Copy link
Contributor

@steveej steveej Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe rename this to queryUserOrGetEnvForPlatform

Copy link
Member

@wking wking Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@steveej steveej Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you'd rather double-check each function where it gets the input from instead of having the information readily available in the name?

Copy link
Member

@wking wking Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

prompt := asset.UserProvided{
Prompt: &survey.Select{
Message: "Platform",
Options: validPlatforms,
},
EnvVarName: "OPENSHIFT_INSTALL_PLATFORM",
}

platform, err := prompt.Generate(nil)
if err != nil {
return "", err
}

return platform
return string(platform.Contents[0].Data), nil
}

func (a *Platform) awsPlatform() (*asset.State, error) {
var region string
survey.AskOne(&survey.Select{
Message: "Region",
Help: "The AWS region to be used for installation.",
Default: "us-east-1 (N. Virginia)",
Options: []string{
"us-east-2 (Ohio)",
"us-east-1 (N. Virginia)",
"us-west-1 (N. California)",
"us-west-2 (Oregon)",
"ap-south-1 (Mumbai)",
"ap-northeast-2 (Seoul)",
"ap-northeast-3 (Osaka-Local)",
"ap-southeast-1 (Singapore)",
"ap-southeast-2 (Sydney)",
"ap-northeast-1 (Tokyo)",
"ca-central-1 (Central)",
"cn-north-1 (Beijing)",
"cn-northwest-1 (Ningxia)",
"eu-central-1 (Frankfurt)",
"eu-west-1 (Ireland)",
"eu-west-2 (London)",
"eu-west-3 (Paris)",
"sa-east-1 (São Paulo)",
prompt := asset.UserProvided{
Prompt: &survey.Select{
Message: "Region",
Help: "The AWS region to be used for installation.",
Default: "us-east-1 (N. Virginia)",
Options: []string{
"us-east-2 (Ohio)",
"us-east-1 (N. Virginia)",
"us-west-1 (N. California)",
"us-west-2 (Oregon)",
"ap-south-1 (Mumbai)",
"ap-northeast-2 (Seoul)",
"ap-northeast-3 (Osaka-Local)",
"ap-southeast-1 (Singapore)",
"ap-southeast-2 (Sydney)",
"ap-northeast-1 (Tokyo)",
"ca-central-1 (Central)",
"cn-north-1 (Beijing)",
"cn-northwest-1 (Ningxia)",
"eu-central-1 (Frankfurt)",
"eu-west-1 (Ireland)",
"eu-west-2 (London)",
"eu-west-3 (Paris)",
"sa-east-1 (São Paulo)",
},
},
}, &region, nil)
EnvVarName: "OPENSHIFT_INSTALL_AWS_REGION",
}
region, err := prompt.Generate(nil)
if err != nil {
return nil, err
}

return assetStateForStringContents(
AWSPlatformType,
strings.Split(region, " ")[0],
strings.Split(string(region.Contents[0].Data), " ")[0],
), nil
}

func (a *Platform) libvirtPlatform() (*asset.State, error) {
var uri string
survey.AskOne(&survey.Input{
Message: "URI",
Help: "The libvirt connection URI to be used. This must be accessible from the running cluster.",
Default: "qemu+tcp://192.168.122.1/system",
}, &uri, nil)
prompt := asset.UserProvided{
Prompt: &survey.Input{
Copy link
Contributor

@steveej steveej Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you forget to use &survey.Select here?

Copy link
Contributor Author

@crawford crawford Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. This is a free-form field.

Copy link
Contributor

@steveej steveej Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@crawford crawford Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Message: "URI",
Help: "The libvirt connection URI to be used. This must be accessible from the running cluster.",
Default: "qemu+tcp://192.168.122.1/system",
},
EnvVarName: "OPENSHIFT_INSTALL_LIBVIRT_URI",
}
uri, err := prompt.Generate(nil)
if err != nil {
return nil, err
}

return assetStateForStringContents(
LibvirtPlatformType,
uri,
string(uri.Contents[0].Data),
), nil
}

Expand Down
8 changes: 8 additions & 0 deletions pkg/asset/installconfig/ssh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@ func (a *sshPublicKey) Dependencies() []asset.Asset {

// 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 {
Copy link
Contributor

@steveej steveej Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@wking wking Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Contributor Author

@crawford crawford Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting OPENSHIFT_INSTALL_SSH_PUB_KEY to an empty string effectively boots without any keys. There is an empty key, but that's okay.

Copy link
Contributor

@steveej steveej Sep 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

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.

Copy link
Member

@wking wking Sep 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return &asset.State{
Contents: []asset.Content{{
Data: []byte(value),
}},
}, nil
}

pubKeys := map[string][]byte{
none: {},
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/asset/installconfig/stock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,34 +55,39 @@ func (s *StockImpl) EstablishStock() {
Message: "Email Address",
Help: "The email address of the cluster administrator. This will be used to log in to the console.",
},
EnvVarName: "OPENSHIFT_INSTALL_EMAIL_ADDRESS",
}
s.password = &asset.UserProvided{
AssetName: "Password",
Prompt: &survey.Password{
Message: "Password",
Help: "The password of the cluster administrator. This will be used to log in to the console.",
},
EnvVarName: "OPENSHIFT_INSTALL_PASSWORD",
}
s.baseDomain = &asset.UserProvided{
AssetName: "Base Domain",
Prompt: &survey.Input{
Message: "Base Domain",
Help: "The base domain of the cluster. All DNS records will be sub-domains of this base.",
},
EnvVarName: "OPENSHIFT_INSTALL_BASE_DOMAIN",
}
s.clusterName = &asset.UserProvided{
AssetName: "Cluster Name",
Prompt: &survey.Input{
Message: "Cluster Name",
Help: "The name of the cluster. This will be used when generating sub-domains.",
},
EnvVarName: "OPENSHIFT_INSTALL_CLUSTER_NAME",
}
s.pullSecret = &asset.UserProvided{
AssetName: "Pull Secret",
Prompt: &survey.Input{
Message: "Pull Secret",
Help: "The container registry pull secret for this cluster.",
},
EnvVarName: "OPENSHIFT_INSTALL_PULL_SECRET",
}
s.platform = &Platform{}
s.sshKey = &sshPublicKey{}
Expand Down
13 changes: 10 additions & 3 deletions pkg/asset/userprovided.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
package asset

import (
"os"

"github.com/AlecAivazis/survey"
)

// UserProvided generates an asset that is supplied by a user.
type UserProvided struct {
AssetName string
Prompt survey.Prompt
AssetName string
Prompt survey.Prompt
EnvVarName string
}

var _ Asset = (*UserProvided)(nil)
Expand All @@ -20,7 +23,11 @@ func (a *UserProvided) Dependencies() []Asset {
// Generate queries for input from the user.
func (a *UserProvided) Generate(map[Asset]*State) (*State, error) {
var response string
survey.AskOne(a.Prompt, &response, survey.Required)
if value, ok := os.LookupEnv(a.EnvVarName); ok {
response = value
} else {
survey.AskOne(a.Prompt, &response, survey.Required)
}

return &State{
Contents: []Content{{
Expand Down