Internalize github.com/docker/machine/libmachine#21647
Internalize github.com/docker/machine/libmachine#21647medyagh merged 6 commits intokubernetes:masterfrom
Conversation
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
6ae4e88 to
130c636
Compare
medyagh
left a comment
There was a problem hiding this comment.
@afbjorklund are we leaving out the vmware and parellles drivers ? if yes, could we import them too ?
In this PR, they are included (with their original LICENSE) But in order to test them, you would probably need to get some CI licenses from the vendor? Only the machine drivers are included, not the actual hypervisor and their control programs... |
|
@afbjorklund we would need to fix the licenses, maybe we can slap our own boilerplate on the top of the file ? could a file have two linceses ? |
This comment was marked as resolved.
This comment was marked as resolved.
Same for the Apache license notices:
|
601d6d1 to
3eafd42
Compare
|
/ok-to-test |
|
/ok-to-test |
|
@afbjorklund the cross build fails, I think we might need to remove building the kvm drivers from the cross build script |
medyagh
left a comment
There was a problem hiding this comment.
@afbjorklund the script that jenkins fails on is
hack/jenkins/minikube_cross_build_and_upload.sh
I think we will need to remove these lines
Why, they are for a different build step? I can't repeat the failure locally, normally.
The only way is by provoking the compiler: $ GOOS="windows" GOARCH="amd64" go build -tags "" -a ./pkg/libmachine/shell/*.go
# command-line-arguments
pkg/libmachine/shell/shell_windows.go:52:6: Detect redeclared in this block
pkg/libmachine/shell/shell.go:17:6: other declaration of DetectSince "shell.go" is supposed to be ignored: // +build !windows
package shellGOOS="windows" GOARCH="amd64" \
go build -tags "" -ldflags="-X k8s.io/minikube/pkg/version.version=v1.37.0 -X k8s.io/minikube/pkg/version.isoVersion=v1.37.0-1760609724-21757 -X k8s.io/minikube/pkg/version.gitCommitID="75d6e112bff2fff720b9eff8eb12aff9a42ec072" -X k8s.io/minikube/pkg/version.storageProvisionerVersion=v5" -a -o out/minikube-windows-amd64 k8s.io/minikube/cmd/minikube |
|
Ah, it was the "boilerplate" copyright script that broke it - by adding itself before the Apparently the script only handles the new-style --- a/pkg/libmachine/shell/shell.go
+++ b/pkg/libmachine/shell/shell.go
@@ -1,3 +1,5 @@
+//go:build !windows
+
/*
Copyright 2022 The Kubernetes Authors All rights reserved.
@@ -14,8 +16,6 @@ See the License for the specific language governing permissions and
limitations under the License.
*/
-// +build !windows
-
package shell
import (I fixed the other places, but didn't test windows. |
3eafd42 to
29e463f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Something is wrong with the existing state:
Possibly related to the old config migration. @@ -117,12 +133,7 @@ func (s Filestore) loadConfig(h *host.Ho
// struct in the migration.
name := h.Name
- migratedHost, migrationPerformed, err := host.MigrateHost(h, data)
- if err != nil {
- return fmt.Errorf("Error getting migrated host: %s", err)
- }
-
- *h = *migratedHost
+ migrationPerformed := false
h.Name = name
|
|
Maybe take a look at the updated libmachine, and include that? |
|
@afbjorklund merged that one, this would we could merge once rebased |
|
More cleanup, removing the unused "crashreport" directory: |
This comment was marked as resolved.
This comment was marked as resolved.
|
I will redo this, once the drivers are merged (and deleted) and after that the provisioners will be deleted too:
When completed, there will only be virtual helper drivers and helper provisioners left under pkg/libmachine: |
Only add libmachine directory, without examples. Don't include docker-machine commands or version.
Change the github.com/docker/machine/libmachine API to internal package k8s.io/minikube/pkg/libmachine. It is no longer available for any external drivers, all drivers are now internal and builtin to minikube.
f5f0858 to
758be21
Compare
758be21 to
efe7659
Compare
|
Split the copying, the boilerplate and the gofmt into separate commits so that they can be ignored while reviewing. The only code change here is in 46066f1, to stop using "mcndirs" and "version". And removing virtualbox example. When compared with https://github.com/minikube-machine/machine (the drivers had already been copied earlier) |
|
/ok-to-test |
|
@afbjorklund: The following tests 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. DetailsInstructions 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. |
The libmachine API is not public anymore, it is minikube private.
To make things easier, move from minikube-machine to minikube:
https://github.com/minikube-machine/machine /libmachine
https://github.com/kubernetes/minikube /pkg/libmachine
Skip the host version v2-v3 migration code and os provisioners.Also skip all the version handling, for downloading boot2docker.
Only copy the hyperv and virtualbox drivers actually being used.build deps: Move hyperv and virtualbox drivers to minikube #21966
Import github.com/Parallels/docker-machine-parallels/v2Drop the "version" since it is not needed anymore.build: Copy the external parallels driver to internal #21968
Import github.com/machine-drivers/docker-machine-driver-vmwareDrop the "config" subdirectory from the module path.Copy the external vmware driver to internal #21969
This PR goes well with dropping hyperkit and internalizing kvm:
Remove the deprecated hyperkit driver #21603
kvm: remove dependency on external driver #21625
Issue #21619
Previous #21637