-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Internalize vendor/github.com/docker/machine #21637
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
Conversation
Skip the host version v0-v2 migration code and os provisioners. Also skip all the version handling, for downloading boot2docker. Only copy the hyperv and virtualbox drivers actually being used. Skip the errdriver and return the binary not found error instead.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: afbjorklund 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the VMware and parallels drivers look good.
libmachine is pretty big, and it seems that some files are duplicating existing minikube files.
The UnsupportedDriver looks interesting, we could use it to simplify the kvm internal driver.
0218fc0
to
4673152
Compare
@afbjorklund: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
@@ -0,0 +1,509 @@ | |||
package hyperv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woud need to add boilerplate copyright here and other new files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that the boilerplate test does not accept the copyright:
Copyright 2014 Docker, Inc.
Copyright 2022 The Kubernetes Authors All rights reserved.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
Even if you do merge it with the kubernetes copyright, and the license.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to keep the docker copyright? This code will quickly change and docker has abandoned this code years ago.
We can add a note that this was imported from the docker project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for taking on this big technical debt,
I am curious does it mean we will stop supporting parallel,vmware ? or they were broken anyways ?
I have no idea, if they actually worked. Just deleted them, since it was less code for me to move that way... But it is not required to remove them, you could add them next to virtualbox if you want to keep them on.
I just thought the idea was to remove virtualbox as well? And hyperkit removed itself, for using vfkit instead. |
It would be nice to do this change without removing the VMware and parallels drivers. But it this means adding even more code from libmachine than it is better to remove them. We have too many drivers and nobody to test or maintain them. Adding more code to support old and possibly unused driver is the wrong direction. It will be useful to have some telemetry so we know which drivers and features are actually used. |
The code is not from libmachine itself, it is from the vendors (possibly through machine-drivers)
The vmware driver actually builds on the original vmware driver that was included with libmachine. |
I think for the old drivers we can keep them and use the old vendor packages until we understand the status better.
If we add this code we must keep this copyright notice. Does it conflict with some linter in minikube?
Any issue add docker copyright in the same way to minikube copyright blog? |
I'm thinking it might be easier to wait with this, until the deprecation period is over and ready to remove? Meanwhile it would be possible to fix the linter issues, by paying some attention to the CI in minikube-machine But at least you know what it would look like, pros and cons. I will cherry-pick the documentation changes to a new PR... EDIT: |
|
Skip the host version v0-v2 migration code and os provisioners.
Also skip all the version handling, for downloading boot2docker.
Only copy the hyperv and virtualbox drivers actually being used.
Skip the errdriver and return the binary not found error instead.
Fixes #21619
Removes the vmware and parallels drivers, since they are not hosted within the minikube monorepo.
The old libmachine code has not been updated or linted or fixed in any way, it is straight off "vendor".
NOTICE:
Copyright 2014 Docker, Inc.
LICENSE:
Apache-2.0
45 issues: