Skip to content
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

pkg/asset/installconfig: add vSphere TUI wizard #3467

Merged

Conversation

patrickdillon
Copy link
Contributor

@patrickdillon patrickdillon commented Apr 15, 2020

This pull request adds the interactive wizard for creating a vSphere install config. After querying credentials from the user, the options are populated from the vCenter API. In cases where only one option is available, it is automatically selected.

This is WIP, but is mostly done, so I am posting for general feedback as well as specific feedback regarding networks. There seem to be four types of vSphere networks. In our vcenter there are DistributedVirtualPortGroups and DistributedVirtualSwitches, and some of these have children. Are all of these valid selections or should I be filtering some of them? The network list is pulled here: https://github.com/openshift/installer/compare/master...patrickdillon:vsphere-tui-cors-1415?expand=1#diff-8366d626249d949112555caeac30a52cR262

Here is a screenshot of the wizard:
Screenshot from 2020-04-15 19-30-06

On my to-do list:

  • add VIPs. This is manual entry and we'll just validate that the form is a valid IP so this is straightforward.
  • Currently if you are unable to login to vCenter the survey loops, continually asking you for username/pw until you can login. I thought this was cool, but survey doesn't work well with CTRL+C so if you CTRL+C trying to get out of this loop you can get an ugly error. I think I will just remove this and exit the installer if you can't login.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 15, 2020
@patrickdillon
Copy link
Contributor Author

/cc @jcpowermac

@jcpowermac
Copy link
Contributor

This looks really good. The only thing from my perspective is the network comment.

@jcpowermac
Copy link
Contributor

@patrickdillon #3470
for dnsvip removal

@patrickdillon patrickdillon changed the title WIP: pkg/asset/installconfig: add vSphere TUI wizard pkg/asset/installconfig: add vSphere TUI wizard Apr 16, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 16, 2020
@patrickdillon
Copy link
Contributor Author

I have addressed the networks issue and outstanding items, so I'm moving this out of WIP and I think it is ready.

@jcpowermac
Copy link
Contributor

FYI: This won't pass /test e2e-vsphere since the dnsvip isn't there. And the changes to either the installer or mco hasn't merged yet.

@patrickdillon
Copy link
Contributor Author

FYI: This won't pass /test e2e-vsphere since the dnsvip isn't there. And the changes to either the installer or mco hasn't merged yet.

e2e-vsphere has an install-config so this PR might incorrectly pass. But that reminds me we should update steps job. I will open a PR.

@jcpowermac
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2020

// Survey does not allow validation of groups of input
// so we perform our own validation.
logrus.Infof("Connecting to vCenter %s", vcenter)
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should move this to debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used Info because there is a noticeable delay for the network connection. This lets the user know what's happening and that the process is not frozen. The pause is very short, a few seconds in my case, so I'm willing to change but wanted to explain the reasoning.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
This commit adds the survey prompts to create an install config.
After querying the user for credentials, the wizard uses the vCenter
API to present choices for all options.
@abhinavdahiya
Copy link
Contributor

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Apr 17, 2020

@patrickdillon: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-scaleup-rhel7 48cc11e link /test e2e-aws-scaleup-rhel7
ci/prow/e2e-openstack 48cc11e link /test e2e-openstack

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 19d22e6 into openshift:master Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants