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

Move supported drivers per platform #4738

Conversation

josedonizetti
Copy link
Member

This is something minishift do and is pretty useful. Wdyt?

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 11, 2019
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

The full list of commands accepted by this bot can be found 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

@afbjorklund
Copy link
Collaborator

To be honest I'm not sure why even bother with plugins, if we are hard-coding the list of "supported" ones anyway ? It's not like you can add your own, since `minikube' will just complain that it is not supported...

But since we are going the way of validating the drivers (#4720), then maybe this makes the code prettier/smaller. And first glance it just looks like duplication, but maybe that can be cleaned up a bit ?

@josedonizetti
Copy link
Member Author

josedonizetti commented Jul 11, 2019

@afbjorklund I don't see it the same way. I understood this supported drivers as something for the documentation, maybe the naming isn't good, but it is simple to change. I think the documentation should be as clean as possible, listing the KVM driver for a Windows user when he runs minikube start --help is misleading, and the idea here is to improve this documentation, not to force the use of only those drivers. That's the case for the PR #4720 as well, it should only validate that if someone tries the none driver in Darwin or Windows he is alerted this won't work, cos it's not for those environments, not that only those drivers are now supported and no one can try to create his own using a plugin.

At the same time, no strong feeling about all this we can close both PRs. :)

@afbjorklund
Copy link
Collaborator

I’m all for filtering out the invalid drivers (i.e. not available on platform)

Copy link
Contributor

@tstromberg tstromberg left a comment

Choose a reason for hiding this comment

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

@minikube-bot OK to test

@josedonizetti josedonizetti force-pushed the move-supported-drivers-per-platform branch from 1362248 to 254dac5 Compare July 15, 2019 18:34
Copy link
Contributor

@blueelvis blueelvis left a comment

Choose a reason for hiding this comment

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

Pretty sure Parallels is not supported on Windows.

@afbjorklund
Copy link
Collaborator

Apparently it got cancelled. https://www.parallels.com/blogs/parallels-desktop-for-windows/

@josedonizetti josedonizetti force-pushed the move-supported-drivers-per-platform branch from 254dac5 to 2ed16a1 Compare July 16, 2019 18:07
@tstromberg tstromberg merged commit fe6e2b1 into kubernetes:master Jul 17, 2019
@josedonizetti josedonizetti deleted the move-supported-drivers-per-platform branch July 17, 2019 16:07
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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants