Skip to content
This repository has been archived by the owner on Feb 6, 2024. It is now read-only.

Fix getting the current cluster name in README.md #214

Merged
merged 1 commit into from
Nov 7, 2018

Conversation

pierreis
Copy link
Contributor

@pierreis pierreis commented Nov 6, 2018

The current documentation suggests using the current context name (provided by kubectl config current-context) as cluster name.

This does not work in configurations in which there is a mismatch between the cluster and context name (_e.g., K8s provided by Docker for Mac).

This pull request fixes this by using the output of kubectl config view --minify -o=jsonpath='{.contexts[0].context.cluster}' instead.

Fixes #213

* Get the actual cluster name from the current context instead of the current context name
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@pierreis
Copy link
Contributor Author

pierreis commented Nov 6, 2018

/assign @fejta

@googlebot
Copy link

CLAs look good, thanks!

@@ -106,7 +106,7 @@ k8s_defaults(
name = "k8s_deploy",
kind = "deployment",
# This is the name of the cluster as it appears in:
# kubectl config current-context
# kubectl config view --minify -o=jsonpath='{.contexts[0].context.cluster}'
Copy link
Contributor

Choose a reason for hiding this comment

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

@nlopezgi
Context is the combination of user + cluster. By default gcloud will set user, cluster and context to be the same thing (probably why this bug exists) but that need not be the case. Totally possible that other distros besides GKE do something different.

Context may actually be what most people want here (and is typically what people use). Probably worth providing a context parameter here in addition to cluster (which is probably better than repurposing cluster to be cluster).

@mattmoor can you comment on the history here? Is this an accident or intentional?

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fejta

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fejta
Copy link
Contributor

fejta commented Nov 6, 2018

/ok-to-test

@fejta
Copy link
Contributor

fejta commented Nov 6, 2018

/retest

1 similar comment
@fejta
Copy link
Contributor

fejta commented Nov 6, 2018

/retest

@fejta
Copy link
Contributor

fejta commented Nov 7, 2018

/assign @smukherj1

Any idea why this keeps failing? Are we out of quota?

@fejta
Copy link
Contributor

fejta commented Nov 7, 2018

/retest

@fejta
Copy link
Contributor

fejta commented Nov 7, 2018

Also @smukherj1 @nlopezgi can you trigger the non prow builds?

@smukherj1
Copy link
Contributor

Opened #216 to hopefully reduce e2e flakiness

@smukherj1 smukherj1 merged commit 0e41295 into bazelbuild:master Nov 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to deploy to local cluster
5 participants