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

Update "parallels" driver library and make this driver built into minikube #9517

Merged
merged 3 commits into from
Oct 28, 2020

Conversation

legal90
Copy link
Contributor

@legal90 legal90 commented Oct 21, 2020

Changes:

Before this PR:

Make sure the docker-machine-driver-parallels is not available:

$ rm /usr/local/bin/docker-machine-driver-parallels
$ which docker-machine-driver-parallels
<empty response>

Then minikube requires user to install it:

$ minikube start --driver parallels
😄  minikube v1.14.0 on Darwin 10.15.7
    ▪ KUBECONFIG=/Users/legal/.kube/config
✨  Using the parallels driver based on existing profile

🤷  Exiting due to PROVIDER_PARALLELS_NOT_FOUND: The 'parallels' provider was not found: exec: "docker-machine-driver-parallels": executable file not found in $PATH
💡  Suggestion: Install docker-machine-driver-parallels
📘  Documentation: https://minikube.sigs.k8s.io/docs/reference/drivers/parallels/

After this PR:

Now the driver is built-in, so no additional action is required form user (as long as Parallels Desktop for Mac is installed and license is activated)

$ minikube start --driver parallels
😄  minikube v1.14.0 on Darwin 10.15.7
    ▪ KUBECONFIG=/Users/legal/.kube/config
✨  Using the parallels driver based on existing profile
👍  Starting control plane node minikube in cluster minikube
💾  Downloading Kubernetes v1.19.2 preload ...
    > preloaded-images-k8s-v6-v1.19.2-docker-overlay2-amd64.tar.lz4: 486.33 MiB
🏃  Updating the running parallels "minikube" VM ...
🐳  Preparing Kubernetes v1.19.2 on Docker 19.03.12 ...
🔎  Verifying Kubernetes components...
🌟  Enabled addons: storage-provisioner, default-storageclass
🏄  Done! kubectl is now configured to use "minikube" by default

cc @sharifelgamal , @afbjorklund , @tstromberg

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 21, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @legal90. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 21, 2020
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@legal90
Copy link
Contributor Author

legal90 commented Oct 21, 2020

/assign @tstromberg

Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

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

thank you for fixing it, I have no way of testing this myself but I trust you have tested this.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2020
Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

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

do you mind putting the PR description the before and after this PR output ?

@codecov-io
Copy link

Codecov Report

Merging #9517 into master will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9517      +/-   ##
==========================================
+ Coverage   29.05%   29.11%   +0.06%     
==========================================
  Files         171      172       +1     
  Lines       10446    10461      +15     
==========================================
+ Hits         3035     3046      +11     
- Misses       6988     6990       +2     
- Partials      423      425       +2     
Impacted Files Coverage Δ
pkg/addons/addons.go 35.26% <0.00%> (ø)
pkg/minikube/driver/install.go 8.95% <0.00%> (ø)
pkg/minikube/driver/version.go 73.33% <0.00%> (ø)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2020
@legal90
Copy link
Contributor Author

legal90 commented Oct 22, 2020

@medyagh Thanks you! Yes, I can add more details and provide "before" and "after" behavior later today.

But since you've merged #9494 I will also need to rebase this PR. Could you please tell me how do you want parallels driver to be called:

  1. via RPC (as a standalone binary) ?
  2. OR as a built-in library (similar to how it's done for virtualbox now) ?

@tstromberg
Copy link
Contributor

I personally think that doing so built-in creates a better user experience, rather than forcing users to download a separate helper binary.

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.

LGTM from my perspective.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: legal90, medyagh, tstromberg

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

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2020
@legal90 legal90 changed the title Update "parallels" driver library, delete the redundant binary check Update "parallels" driver library and make this driver built into minikube Oct 23, 2020
@legal90
Copy link
Contributor Author

legal90 commented Oct 23, 2020

@tstromberg @medyagh I rebased the branch, updated the PR title and description.

The idea is still the same - this PR makes parallels driver built-in, so no separate driver binary is required anymore (so that, it reverts #9494).
Also, it updates the driver to the latest version, which fixes #9357

@tstromberg
Copy link
Contributor

Thank you!

@tstromberg tstromberg merged commit c462ac8 into kubernetes:master Oct 28, 2020
@legal90 legal90 deleted the fix-parallels-library branch October 28, 2020 14:56
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
6 participants