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

Identify minikube cluster for any profile name #4701

Merged
merged 9 commits into from
Aug 28, 2020

Conversation

gsquared94
Copy link
Contributor

@gsquared94 gsquared94 commented Aug 19, 2020

Fixes #3668

Description

Currently we need to explicitly specify the minikube profile name as a skaffold flag or limit it to only 'minikube'. This PR introduces several checks for detecting a minikube cluster so that the flag will no longer be required and we can name the cluster anything.
We check:

  • The context is 'minikube', or
  • The k8s nodes have a minikube label (refer label minikube nodes kubernetes/minikube#6717), or
  • The cluster certificate path is within the ~/.minikube directory (refer this comment), or
  • The minikube profile list returns a config name matching the context, and
  • If the minikube driver is a VM then the node IP should also match the k8s api server url

@gsquared94 gsquared94 requested a review from a team as a code owner August 19, 2020 17:40
@gsquared94 gsquared94 changed the title Detect minikube cluster for any profile name Identify minikube cluster for any profile name Aug 19, 2020
@gsquared94 gsquared94 requested a review from medyagh August 19, 2020 17:46
@codecov
Copy link

codecov bot commented Aug 19, 2020

Codecov Report

Merging #4701 into master will decrease coverage by 0.02%.
The diff coverage is 60.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4701      +/-   ##
==========================================
- Coverage   73.42%   73.39%   -0.03%     
==========================================
  Files         343      344       +1     
  Lines       13560    13669     +109     
==========================================
+ Hits         9957    10033      +76     
- Misses       2980     3002      +22     
- Partials      623      634      +11     
Impacted Files Coverage Δ
pkg/skaffold/kubernetes/context/context.go 71.42% <0.00%> (-13.94%) ⬇️
pkg/skaffold/docker/client.go 82.95% <12.50%> (+3.57%) ⬆️
pkg/skaffold/util/util.go 86.07% <60.00%> (-0.86%) ⬇️
pkg/skaffold/cluster/minikube.go 68.75% <68.75%> (ø)
pkg/skaffold/config/util.go 69.31% <100.00%> (ø)
cmd/skaffold/app/skaffold.go 83.33% <0.00%> (-16.67%) ⬇️
pkg/skaffold/deploy/kpt.go 84.27% <0.00%> (+1.41%) ⬆️

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 f54a62b...23d9edd. Read the comment docs.

gsquared94 added a commit to gsquared94/skaffold that referenced this pull request Aug 19, 2020
Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

Please create follow issue to deprecate --minikube-profile #3668

Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

few nit changes to simply the code

pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
@gsquared94
Copy link
Contributor Author

few nit changes to simply the code

Hopefully addressed all of them.

@gsquared94
Copy link
Contributor Author

Please create follow issue to deprecate --minikube-profile #3668

Done. #4709

Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

Querying the API server is just too expensive — we have some users developing against a cluster on the other side of the world. I thought @tstromberg's suggestion was to load up the kubeconfig files and look at the corresponding cluster's certificate-authority to see if it was in ~/.minikube:

    certificate-authority: /Users/bdealwis/.minikube/ca.crt

pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Show resolved Hide resolved
@gsquared94
Copy link
Contributor Author

gsquared94 commented Aug 27, 2020

Querying the API server is just too expensive — we have some users developing against a cluster on the other side of the world. I thought @tstromberg's suggestion was to load up the kubeconfig files and look at the corresponding cluster's certificate-authority to see if it was in ~/.minikube:

    certificate-authority: /Users/bdealwis/.minikube/ca.crt

@briandealwis, I implemented this approach and removed the label checker. Although the label checker was a very deterministic check and we could have cached the result to not have to run it multiple times. Maybe we can revisit if current set of heuristics don't prove sufficient.

@gsquared94 gsquared94 requested a review from nkubala August 27, 2020 14:55
Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

one nit.
And follow up with a PR for parsing error codes.

pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Outdated Show resolved Hide resolved
@gsquared94 gsquared94 merged commit 67e81bc into GoogleContainerTools:master Aug 28, 2020
Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

A I noticed a few issues that should be fixed.

pkg/skaffold/cluster/minikube.go Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Show resolved Hide resolved
pkg/skaffold/cluster/minikube.go Show resolved Hide resolved
@gsquared94
Copy link
Contributor Author

A I noticed a few issues that should be fixed.

Fixed them in follow up PR #4742

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable skaffold to work on minikube profiles
5 participants