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

Added update-context and kubeconfig to status #1578

Merged
merged 1 commit into from
Jun 14, 2017

Conversation

abbytiz
Copy link
Contributor

@abbytiz abbytiz commented Jun 12, 2017

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 12, 2017
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@r2d4
Copy link
Contributor

r2d4 commented Jun 12, 2017

@minikube-bot OK to test

I also added your account to the whitelist, so tests should run on all of your PRs automatically

@codecov-io
Copy link

codecov-io commented Jun 12, 2017

Codecov Report

Merging #1578 into master will decrease coverage by 0.44%.
The diff coverage is 23.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1578      +/-   ##
==========================================
- Coverage   39.52%   39.08%   -0.45%     
==========================================
  Files          50       51       +1     
  Lines        2548     2633      +85     
==========================================
+ Hits         1007     1029      +22     
- Misses       1375     1428      +53     
- Partials      166      176      +10
Impacted Files Coverage Δ
pkg/minikube/cluster/cluster.go 44.44% <0%> (-1.52%) ⬇️
cmd/minikube/cmd/status.go 11.36% <0%> (-5.31%) ⬇️
cmd/minikube/cmd/update-context.go 10% <10%> (ø)
pkg/minikube/kubeconfig/config.go 53.33% <45%> (-2.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ab1566...5778163. Read the comment docs.

Copy link
Contributor

@r2d4 r2d4 left a comment

Choose a reason for hiding this comment

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

Cool! Everyone else got to review already so I added a few comments. Looks good though

Long: `Retrieves the IP address of the running cluster, checks it
with IP in kubeconfig, and corrects kubeconfig if incorrect.`,
Run: func(cmd *cobra.Command, args []string) {
api, err := machine.NewAPIClient(clientType)
Copy link
Contributor

Choose a reason for hiding this comment

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

Once #1544 is merged, this will become machine.NewAPIClient()

if kip.Equal(ip) {
return "Correctly Configured: pointing to minikube-vm at " + kip.String(), nil
}
return "Misconfigured: pointing to stale minikube-vm at " + kip.String() +
Copy link
Contributor

Choose a reason for hiding this comment

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

For GetKubeConfigStatus and UpdateKubeconfigIP I don't think we should be returning a user-facing string here.

For GetKubeConfigStatus, you might want to make this return a CheckKubeConfigStatus(ip, filename) error and then move these printed strings further up the stack into the command.

}

// UpdateKubeconfigIP overwrites the IP stored in kubeconfig with the provided IP.
func UpdateKubeconfigIP(ip net.IP, filename string) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For this function, you might want to change the signature to be func UpdateKubeconfigIP(ip, filename) (bool, error) where bool signifies whether or not the kubeconfig was rewritten. And similarly move the printed strings further up the stack in the command.

if err != nil {
return "", errors.Wrap(err, "Error getting kubeconfig status")
}
con.Clusters["minikube"].Server = "https://" + ip.String() + ":" + strconv.Itoa(constants.APIServerPort)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a comment here on why this lookup is safe (since getIPFromKubeConfig ensures that the entry exists)?

description: "no minikube cluster",
ip: net.ParseIP("192.168.10.100"),
existing: fakeKubeCfg,
status: "Kubeconfig does not have a record of a minikube cluster",
Copy link
Contributor

Choose a reason for hiding this comment

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

Once you change the statuses to not be hardcoded strings then these checks will be much cleaner and robust. You'll lose some of the specificity of why the error occurred, but I think thats ok since it should be clear from the test.

}

// getIPFromKubeConfig returns the IP address stored for minikube in the kubeconfig specified
func getIPFromKubeConfig(filename string) (net.IP, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@aaron-prindle does this need to be parameterized with machine name instead of minikube for the --profile flag?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. @abbytiz minikube allows for multiple machines to be named. The name 'minikube' is the default but it can hold any value. You can test that your code is working with this by launching minikube using 'minikube start --profile test'. This will spin up a second copy of minikube with its own name and kubeconfig using that name.

@@ -165,6 +165,24 @@ func GetLocalkubeStatus(api libmachine.API) (string, error) {
}
}

// GetHostDriverIP gets the ip address of the current minikube cluster
func GetHostDriverIP(api libmachine.API) (net.IP, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I can't believe we didn't have something like this before. Out of scope for this PR, but we might want to consider moving some of our other functions that just call Driver.GetIP() directly to use this, so we can pass around a real net.IP struct.

if err != nil {
return "", errors.Wrap(err, "Error getting kubeconfig status")
}
con.Clusters["minikube"].Server = "https://" + ip.String() + ":" + strconv.Itoa(constants.APIServerPort)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is safe to use a hardcoded 'minikube' here. The value here comes from the machineName. You can see how the kubeconfig is created in start.go using the machineName here: https://github.com/kubernetes/minikube/blob/master/cmd/minikube/cmd/start.go#L192

@aaron-prindle
Copy link
Contributor

@minikube-bot retest this please

@aaron-prindle
Copy link
Contributor

This works great locally. Just squash this down into a singe commit and then we can get it merged. Nice job!

@aaron-prindle aaron-prindle merged commit e524e8a into kubernetes:master Jun 14, 2017
@aaron-prindle
Copy link
Contributor

LGTM, merging. Perhaps as an additional PR we can have update-context set the kubectl context to use minikube as doing so through kubectl seems to trip up some users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants