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

Adds 'context' parameter to k8s_object #138

Merged
merged 5 commits into from
Jul 23, 2018
Merged

Conversation

jasongwartz
Copy link

Apologies for opening this a second time. I had to rebase in order to get the right email address in the commit (for the CLA).

Related: #135

This PR introduces a new parameter to k8s_object, in order to manually specify the 'context' from the local kubeconfig file.
This is necessary in addition to the 'cluster' parameter in cases where the bazel rule should be applied into a 'context' which is configured on the local machine, but not currently selected.

For example: in our use case, we have a macro which generates and runs k8s_object rules for all of our deploy environments and clusters (dev, prod, multiple clusters) - which means it needs to run apply against different 'contexts' consecutively without changing the kubectl context.

cc @Globegitter

* Adds 'context' parameter to k8s_object

This commit introduces a new parameter to k8s_object, in order
to manually specify the 'context' from the local kubeconfig file.
This is necessary in addition to the 'cluster' parameter in cases
where the bazel rule should be applied into a 'context' which is
configured on the local machine, but not currently selected.

* Adds clarifying comment about default behaviour

* Adds context to all other rules in addition to apply
@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

@jasongwartz
Copy link
Author

I signed it!

(I believe my company has signed)

@jasongwartz
Copy link
Author

Ok, NOW it should confirmed signed.

@googlebot
Copy link

CLAs look good, thanks!

@fejta
Copy link
Contributor

fejta commented Jun 26, 2018

Does your kubectl config get-contexts differ from kubectl config get-clusters?

k8s/object.bzl Outdated

# If the 'context' parameter is not set by the caller,
# this value becomes an empty string, and kubectl
# will be run like `kubectl --context= ...` In this
Copy link
Contributor

Choose a reason for hiding this comment

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

Would want this to be kubectl --context={cluster} instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

No it seems like --context= is better

@fejta fejta mentioned this pull request Jun 26, 2018
@fejta
Copy link
Contributor

fejta commented Jun 27, 2018

/assign

@jasongwartz
Copy link
Author

@fejta Well - in my personal .kube/config, the context names are the same as the cluster names, but I have, in the past, set 'friendly' context names for clusters with very similar names (in combination with using https://github.com/ahmetb/kubectx).

In fact, the first tests of this PR didn't add an extra bazel parameter and just modified the kubectl command adding --context={cluster}, but I decided it was safer to accommodate more generalised use-cases, hence being able to set {cluster} and {context} independently.

I hope I'm understanding your comment correctly!

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

k8s/object.bzl Outdated

# If the 'context' parameter is not set by the caller,
# this value becomes an empty string, and kubectl
# will be run like `kubectl --context= ...` In this
Copy link
Contributor

Choose a reason for hiding this comment

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

No it seems like --context= is better

@jasongwartz
Copy link
Author

@fejta Great, thanks for reviewing!

I'm not sure what else needs to be done, or if an owner like you just needs to press the button.

@fejta
Copy link
Contributor

fejta commented Jul 3, 2018

I'm not sure what else needs to be done

I need this to pass e2e tests. There's an issue where travis cannot test PRs from other repos. I'm fixing this by migrating to prow.

I'll merge the PR once this is done.

Thanks very much for your patience!

@jasongwartz
Copy link
Author

Ok, no problem!

@fejta fejta closed this Jul 9, 2018
@fejta fejta reopened this Jul 9, 2018
@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fejta, jasongwartz

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 Jul 9, 2018

/ok-to-test

@fejta
Copy link
Contributor

fejta commented Jul 18, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 23, 2018
@k8s-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@nlopezgi
Copy link
Contributor

/test pull-rules-k8s-e2e

@fejta
Copy link
Contributor

fejta commented Jul 23, 2018

/override cla/google

@k8s-ci-robot
Copy link

@fejta: Overrode contexts on behalf of fejta: cla/google

In response to this:

/override cla/google

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.

@fejta fejta merged commit 26a8199 into bazelbuild:master Jul 23, 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.

6 participants