Skip to content

asset/*: re-implement user prompts #287

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 4 commits into from
Sep 20, 2018
Merged

asset/*: re-implement user prompts #287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a607e54
vendor: clean vendor directory
crawford Sep 19, 2018
d220b1d
vendor: clean glide state
crawford Sep 19, 2018
37c6173
vendor: add github.com/AlecAivazis/survey
crawford Sep 19, 2018
329b927
asset/*: re-implement user prompts
crawford Sep 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
20 changes: 17 additions & 3 deletions glide.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions glide.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import:
version: 0.0.0-alpha.2
subpackages:
- pkg/client/clientset_generated/clientset
- package: github.com/AlecAivazis/survey
version: 1.6.2
testImport:
- package: github.com/stretchr/testify
version: ^1.2.2
Expand Down
6 changes: 4 additions & 2 deletions pkg/asset/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,17 @@ go_library(
],
importpath = "github.com/openshift/installer/pkg/asset",
visibility = ["//visibility:public"],
deps = ["//vendor/github.com/Sirupsen/logrus:go_default_library"],
deps = [
"//vendor/github.com/AlecAivazis/survey:go_default_library",
"//vendor/github.com/Sirupsen/logrus:go_default_library",
],
)

go_test(
name = "go_default_test",
srcs = [
"state_test.go",
"store_test.go",
"userprovided_test.go",
],
embed = [":go_default_library"],
deps = ["//vendor/github.com/stretchr/testify/assert:go_default_library"],
Expand Down
6 changes: 2 additions & 4 deletions pkg/asset/installconfig/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ go_library(
"//pkg/asset:go_default_library",
"//pkg/ipnet:go_default_library",
"//pkg/types:go_default_library",
"//vendor/github.com/AlecAivazis/survey:go_default_library",
"//vendor/github.com/apparentlymart/go-cidr/cidr:go_default_library",
"//vendor/github.com/ghodss/yaml:go_default_library",
"//vendor/github.com/pborman/uuid:go_default_library",
Expand All @@ -26,10 +27,7 @@ go_library(

go_test(
name = "go_default_test",
srcs = [
"installconfig_test.go",
"platform_test.go",
],
srcs = ["installconfig_test.go"],
embed = [":go_default_library"],
deps = [
"//pkg/asset:go_default_library",
Expand Down
64 changes: 46 additions & 18 deletions pkg/asset/installconfig/platform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
package installconfig

import (
"bufio"
"fmt"
"strings"

"github.com/AlecAivazis/survey"

"github.com/openshift/installer/pkg/asset"
)

Expand All @@ -17,7 +18,6 @@ const (

var (
validPlatforms = []string{AWSPlatformType, LibvirtPlatformType}
platformPrompt = fmt.Sprintf("Platform (%s)", strings.Join(validPlatforms, ", "))
)

// Platform is an asset that queries the user for the platform on which to install
Expand All @@ -30,9 +30,7 @@ var (
//
// * Libvirt
// Contents[1] is the URI.
type Platform struct {
InputReader *bufio.Reader
}
type Platform struct{}

var _ asset.Asset = (*Platform)(nil)

Expand Down Expand Up @@ -60,30 +58,60 @@ func (a *Platform) Name() string {
}

func (a *Platform) queryUserForPlatform() string {
for {
input := asset.QueryUser(a.InputReader, platformPrompt)
input = strings.ToLower(input)
for _, p := range validPlatforms {
if input == p {
return p
}
}
fmt.Println("Invalid platform")
}
var platform string
survey.AskOne(&survey.Select{
Message: "Platform",
Options: validPlatforms,
}, &platform, nil)

return platform
}

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)",
Copy link
Member

@wking wking Sep 20, 2018

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?

Copy link
Contributor Author

@crawford crawford Sep 20, 2018

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.

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

return assetStateForStringContents(
AWSPlatformType,
asset.QueryUser(a.InputReader, "Region"),
strings.Split(region, " ")[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)

return assetStateForStringContents(
LibvirtPlatformType,
// TODO(yifan): Set the default URI.
asset.QueryUser(a.InputReader, "URI"),
uri,
), nil
}

Expand Down
79 changes: 0 additions & 79 deletions pkg/asset/installconfig/platform_test.go
View file
Open in desktop

This file was deleted.

Loading