Skip to content

Conversation

@jcpowermac
Copy link
Contributor

@jcpowermac jcpowermac commented Jan 24, 2020

This PR requires #2922

Used @jstuever branch as the basis of this change.

import.go implements the ability for the installer to
import an RHCOS OVA into vSphere.
findImportOvaParams will only provide a HostSystem
that contains the Network and Datastore provided for
the import process.

Used govc/importx/archive.go which is required
to retrieve the Ovf Envelope

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 24, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign abhinavdahiya
You can assign the PR to them by writing /assign @abhinavdahiya in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details 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

@jcpowermac jcpowermac changed the title [WIP] Add import [WIP] Add import ova for vSphere Jan 24, 2020
@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Jan 24, 2020

bcd5c62

Most of this should be made private or internal package imo.

Also this changeset would benefit a lot from from comments/internal doc/commit message to explain the process of importing/creating a VM template.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2020
This change caches a copy of the vmware ova specified in rhcos.json in
preperation to upload the OVA into a Template.
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2020
Used @jstuever branch as the basis of this change.

`import.go` implements the ability for the installer to
import an RHCOS OVA into vSphere.
`findImportOvaParams` will only provide a HostSystem
that contains the Network and Datastore provided for
the import process.

Used `govc/importx/archive.go` which is required
to retrieve the Ovf Envelope
@jcpowermac jcpowermac changed the title [WIP] Add import ova for vSphere Add import ova for vSphere Jan 24, 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 Jan 24, 2020
@openshift-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
ci/prow/e2e-openstack f73b77a link /test e2e-openstack
ci/prow/e2e-ovirt f73b77a link /test e2e-ovirt
ci/prow/e2e-aws f73b77a link /test e2e-aws
ci/prow/e2e-aws-fips f73b77a link /test e2e-aws-fips
ci/prow/e2e-aws-scaleup-rhel7 f73b77a link /test e2e-aws-scaleup-rhel7
ci/prow/e2e-aws-upgrade f73b77a link /test e2e-aws-upgrade
ci/prow/e2e-libvirt f73b77a link /test e2e-libvirt

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.

Details

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.

Copy link
Contributor

@patrickdillon patrickdillon left a comment

Choose a reason for hiding this comment

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

I just gave this a quick review, but I have some questions around the vendor/copy of govmomi code.


baseURL := base.ResolveReference(image).String()

// TODO: Get Uncompressed SHA256 into rhcos.json
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this contingent on? If this seems like a lot of work, I think delete the comment and create a card.

return &ArchiveFlag{}, ctx
}

// Register ArchiveFlag
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the only thing being changed from the govmomi repo is adding some comments. These are exported types so I would think we should be able to use these just from the vendored code and not copy here...

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a technical reason... the things we need from the vendored code is not exportable. Therefore, we copied file(s) here with a modified package vsphere thereby making them accessible.

@openshift-ci-robot
Copy link
Contributor

@jcpowermac: PR needs rebase.

Details

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.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 24, 2020
@jcpowermac
Copy link
Contributor Author

/close

@openshift-ci-robot
Copy link
Contributor

@jcpowermac: Closed this PR.

Details

In response to this:

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants