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

Add new VMware unified driver to supported list #2606

Closed
wants to merge 2 commits into from
Closed

Add new VMware unified driver to supported list #2606

wants to merge 2 commits into from

Conversation

frapposelli
Copy link
Member

This PR adds the new VMware unified driver to the supported list, the driver now supports Fusion and Workstation across Windows, Linux and macOS.

A deprecation note has been added for the bundled vmwarefusion driver.

Ref: #2118 #2526 machine-drivers/discussion#5 machine-drivers/docker-machine-driver-vmware#1

Fixes: machine-drivers/docker-machine-driver-vmware#1 #2118

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 15, 2018
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 15, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: frapposelli
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: luxas

Assign the PR to them by writing /assign @luxas in a comment when ready.

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

@frapposelli
Copy link
Member Author

/assign @luxas

@frapposelli
Copy link
Member Author

@dlorenc
Copy link
Contributor

dlorenc commented Mar 19, 2018

We shouldn't really add this to an officially supported list until we get automated integration testing for it.

@minikube-bot OK to test

@frapposelli
Copy link
Member Author

@dlorenc we’re working on getting a CI environment to test integration under the driver repo.

@gbraad
Copy link
Contributor

gbraad commented Mar 19, 2018

@frapposelli glad to hear about that last part... :-D

@frapposelli
Copy link
Member Author

@dlorenc @gbraad it’s going to take some time to have the environment provisioned, in the meantime, this driver is fixing several issues with our platforms, and extending support to Windows and Linux.

Adding support for this and deprecating the docker-machine builtin one should be a priority in order to have better supportability and timely fixes.

@dlorenc
Copy link
Contributor

dlorenc commented Mar 19, 2018

@frapposelli I definitely understand that, but I'm going to have to stay firm on this one. The vmware fusion driver shouldn't have been added without CI tests in the first place, and it's much harder to delete something once it's been merged in.

Once we get CI running, we can add this driver.

@gbraad
Copy link
Contributor

gbraad commented Mar 19, 2018

I also understand, but have to agree with @dlorenc.

@frapposelli
Copy link
Member Author

Understand, let's leave this open and I'll try to speed up with the CI infra on our side.

@dlorenc
Copy link
Contributor

dlorenc commented Apr 11, 2018

Any updates on getting this into the test infrastructure?

@frapposelli
Copy link
Member Author

@dlorenc we’re still waiting on the capacity we requested. Shouldn’t take long

@gbraad
Copy link
Contributor

gbraad commented May 1, 2018

any updates?

@frapposelli
Copy link
Member Author

Yes, we have secured the capacity and we're working on setting up CI, this week there's Kubecon (pretty much our the entire team is here) so I would expect to have something ready by end of next week.

@fenos
Copy link

fenos commented Jun 29, 2018

What's the progress on this?

@frapposelli
Copy link
Member Author

Unfortunately the CI work was re-prioritized a few times, we’re now working to have the vSphere capacity added to the tide pool, after that is done we can run tests and merge this.

@fenos
Copy link

fenos commented Jul 2, 2018

@frapposelli Thanks for the update!
No worries it happens! I Can't wait for having VMWare fusion working properly on minikube, at the current state it doesn't even start a cluster on it 😭

Keep up the great job 🎉

@smebberson
Copy link

Any news on this?

@frapposelli
Copy link
Member Author

Sorry guys, this is taking a little longer than expected.

/cc @akutz FYI

@k8s-ci-robot
Copy link
Contributor

@frapposelli: GitHub didn't allow me to request PR reviews from the following users: FYI.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Sorry guys, this is taking a little longer than expected.

/cc @akutz FYI

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.

Copy link
Contributor

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

Also need to update pkg/minikube/constants/constants.go to add vmware to SupportedVMDrivers.

The changes to the vendor should be backed out.

@RA489
Copy link

RA489 commented Oct 23, 2018

@frapposelli This PR needs rebase.

@tstromberg
Copy link
Contributor

Do you mind addressing the merge conflicts? I'd like to see about merging this if it's still viable. My apologies for the unnecessarily long delay in reviewing this PR.

@frapposelli
Copy link
Member Author

@tstromberg thanks, let me look into it

@frapposelli
Copy link
Member Author

@tstromberg the original fork was deleted, created #3534 as a follow up from this PR, which I'm closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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.

None yet