Welcome @kppullin!

It looks like this is your first PR to kubernetes/minikube 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/minikube has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

Hi @kppullin. 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.

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kppullin
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.

@kppullin thank you for this PR, have you tested this yourself ?
is there a way you could screenshot or paste the output using fish ?

Can one of the admins verify this patch?

Codecov Report

Merging #6744 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6744   +/-   ##
=======================================
  Coverage   38.48%   38.48%           
=======================================
  Files         142      142           
  Lines        8684     8684           
=======================================
  Hits         3342     3342           
  Misses       4923     4923           
  Partials      419      419

Related to 5d59e83 and #6595 ?

Probably needed upstream (libmachine) as well, if eval is indeed "broken" (not working) in fish

https://github.com/docker/machine/blob/v0.16.1/commands/env.go#L132_L135

@medyagh : Should be enough to spawn a new fish shell it now looks totally broken though.

Note that minikube cleverly adds the comment at the end of the env output.

So it doesn't have the same issue, but still broken after semicolon was removed

@afbjorklund I think you're correct that the semicolon removal in 5d59e83 is the "breaking" change.

With the semicolons fish is able to handle the single line chain of commands just fine:

> eval (set -gx DOCKER_TLS_VERIFY "1"; set -gx DOCKER_HOST "tcp://192.168.39.222:2376"; set -gx DOCKER_CERT_PATH "/home/kevin/.minikube/certs"; set -gx 
> env | grep DOCKER
MINIKUBE_ACTIVE_DOCKERD "minikube")`
DOCKER_CERT_PATH=/home/kevin/.minikube/certs
DOCKER_HOST=tcp://192.168.39.222:2376
DOCKER_TLS_VERIFY=1
MINIKUBE_ACTIVE_DOCKERD=minikube

Without the semicolons it just ends up being one long value:

> eval (DOCKER_TLS_VERIFY=1 set -gx DOCKER_HOST tcp://192.168.39.222:2376 set -gx DOCKER_CERT_PATH /home/kevin/.minikube/certs set -gx MINIKUBE_ACTIVE_DOCKERD minikube
MINIKUBE_ACTIVE_DOCKERD=minikube)
> env | grep DOCKER
DOCKER_TLS_VERIFY=1 set -gx DOCKER_HOST tcp://192.168.39.222:2376 set -gx DOCKER_CERT_PATH /home/kevin/.minikube/certs set -gx MINIKUBE_ACTIVE_DOCKERD minikube
MINIKUBE_ACTIVE_DOCKERD=minikube

Reading a bit more documentation, and I'm by far not an expert here, it appears that eval removes the line breaks in both bash and fish. However, since bash's export NAME=VALUE syntax doesn't allow spaces, it ends up treating each export as a separate command.

> /usr/bin/bash
> export A=B export C=D
> env | grep A=B
A=B
> env | grep C=D
C=D

Whereas fish's syntax allows for spaces, yielding:

> /usr/bin/fish
> set -gx A B set -gx C D
> env | grep A=B
A=B set -gx C D

So, considering the options, is it better to:

• Reinsert the semicolons (and hope they're not removed in the future or the comments moved to the top of the output)?
• Change the hint to direct towards piping to source (but being a less conventional/common syntax)?
• Both/Other?

@medyagh yes sorry for not including that yet. Here's the output:

> env | grep DOCKER
> ./out/minikube-linux-amd64 docker-env
set -gx DOCKER_TLS_VERIFY "1"
set -gx DOCKER_HOST "tcp://192.168.39.222:2376"
set -gx DOCKER_CERT_PATH "/home/kevin/.minikube/certs"
set -gx MINIKUBE_ACTIVE_DOCKERD "minikube"

# To point your shell to minikube's docker-daemon, run:
# minikube -p minikube docker-env | source
> ./out/minikube-linux-amd64 -p minikube docker-env | source
> env | grep DOCKER
DOCKER_CERT_PATH=/home/kevin/.minikube/certs
DOCKER_HOST=tcp://192.168.39.222:2376
DOCKER_TLS_VERIFY=1
MINIKUBE_ACTIVE_DOCKERD=minikube
> docker images
REPOSITORY                                TAG                 IMAGE ID            CREATED             SIZE
k8s.gcr.io/kube-proxy                     v1.17.3             ae853e93800d        11 days ago         116MB
k8s.gcr.io/kube-controller-manager        v1.17.3             b0f1517c1f4b        11 days ago         161MB
k8s.gcr.io/kube-apiserver                 v1.17.3             90d27391b780        11 days ago         171MB
k8s.gcr.io/kube-scheduler                 v1.17.3             d109c0821a2b        11 days ago         94.4MB
kubernetesui/dashboard                    v2.0.0-beta8        eb51a3597525        2 months ago        90.8MB
k8s.gcr.io/coredns                        1.6.5               70f311871ae1        3 months ago        41.6MB
k8s.gcr.io/etcd                           3.4.3-0             303ce5db0e90        4 months ago        288MB
kubernetesui/metrics-scraper              v1.0.2              3b08661dc379        4 months ago        40.1MB
k8s.gcr.io/pause                          3.1                 da86e6ba6ca1        2 years ago         742kB
gcr.io/k8s-minikube/storage-provisioner   v1.8.1              4689081edb10        2 years ago         80.8MB

@kppullin I installed fish on mac

and for me both of them works

minikube -p minikube docker-env | source
and
eval (minikube -p minikube docker-env)

however !!! I realized in my fish on mac we fail to detect that it is fish and we show the hint for bash

 ~/minikube (metric_test)> ./out/minikube docker-env
export DOCKER_TLS_VERIFY="1"
export DOCKER_HOST="tcp://127.0.0.1:32778"
export DOCKER_CERT_PATH="/Users/medmac/.minikube/certs"
export MINIKUBE_ACTIVE_DOCKERD="minikube"

# To point your shell to minikube's docker-daemon, run:
# eval $(minikube -p minikube docker-env)

@kppullin do you mind sharing the output of docker-env from your fish ?

@kppullin : I was mostly basing that on that nobody else complained over the (five) years.

docker/machine@f81870e

docker/machine@b5927f1

But then again nobody is using fish, and not much has happened in machine since then...

@medyagh : please double-check that SHELL is pointing to /usr/local/bin/fish

Unless changing your login shell (chsh), I don't think the variable will be updated properly