WIP: Check if minikube is already running #3293
Conversation
k8s-ci-robot
added
the
size/S
|
Can one of the admins verify this patch? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: RA489, ravsa If they are not already assigned, you can assign the PR to them by writing 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 |
|
@RA489: changing LGTM is restricted to assignees, and only kubernetes/minikube repo collaborators may be assigned issues. In response to this:
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. |
ravsa
changed the title
|
@tstromberg, @balopat, @aaron-prindle Please take a look here |
|
@minikube-bot OK to test |
|
Can you explain the purpose of this change? Start is supposed to be safe to call repeatedly. |
|
@minikube-bot OK to test |
balopat
changed the title
k8s-ci-robot
added
the
cncf-cla: yes
|
@minikube-bot OK to test |
ravsa
force-pushed
the
dev-minikube
branch
3 times, most recently
from
5dd4844
to
603b65d
Compare
|
@balopat, could you help me with this build issue? it's taking too long. |
|
@minikube-bot OK to test |
cmd/minikube/cmd/start.go
Outdated
| @@ -112,6 +115,7 @@ func runStart(cmd *cobra.Command, args []string) { | |||
| } | |||
|
|
|||
| api, err := machine.NewAPIClient() | |||
| ensureNotRunning(api, constants.DefaultMachineName) | |||
There was a problem hiding this comment.
What happens if the already running instance has a different name? (like via the --profile argument to minikube start)
There was a problem hiding this comment.
nice catch, I will make the changes. Thanks
|
@minikube-bot OK to test |
1 similar comment
|
@minikube-bot OK to test |
|
I believe this PR breaks the workflow where folks make changes to their local configuration, such as updating the version of Kubernetes used, and use I'm tempted to close this PR - but perhaps I'm not interpreting this change correctly? |
|
I've been thinking more about this, and have a few thoughts:
We need to bridge the gap between the two, and I think the only way to do so is to compare the old configuration against the new one. If they match, exit early, otherwise, go through the full process. Any opinions here? |
|
@tstromberg A good idea is to have 2 different "commands"/"flags" to serve these conflicting needs f.e a |
How about adding a In an ideal world, minikube should be able to detect whether or not reconfiguration is required, but it doesn't yet (PR's welcome!). This flag would allow users to override that detection, and specify whether or not to return quickly - as your PR does - or to forcibly reconfigure. For your solution, you would add |
tstromberg
added
the
needs-rebase
tstromberg
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
I'm closing as this PR seems to be inactive, and having unmergeable PR's around slows us down from making progress on the PR's that are active. Please re-open this PR once you've had a chance to address the feedback on it. Rather than adding extra knobs, my preference is for this PR to check if the requested configuration is different than the previously setup configuration. If it's different, take the slow path - if it's the same, take the fast path. Thank you for proposing this PR. It has sparked a very important discussion. |
Fixes: #2646