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

Kubectl via toolchains #210

Merged
merged 10 commits into from
Nov 1, 2018
Merged

Kubectl via toolchains #210

merged 10 commits into from
Nov 1, 2018

Conversation

alex1545
Copy link
Contributor

Pass value of toolchain attribute that is the default kubectl path to templates.

@alex1545
Copy link
Contributor Author

/assign @erain

@alex1545
Copy link
Contributor Author

/unassign @erain

@alex1545
Copy link
Contributor Author

/cc @erain

@k8s-ci-robot k8s-ci-robot requested a review from erain October 31, 2018 14:19
@alex1545
Copy link
Contributor Author

/cc @xingao267

nlopezgi
nlopezgi reviewed Oct 31, 2018
View reviewed changes
Copy link
Contributor

@nlopezgi nlopezgi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think you might need a default_osx_toolchain to get mac tests to pass (you might also want to add a windows one, though I dont even know if kubectl exists for windows)

@chrislovecnm
Copy link
Contributor

@nlopezgi yes kubectl for windows exists

nlopezgi
nlopezgi reviewed Oct 31, 2018
View reviewed changes
Copy link
Contributor

@nlopezgi nlopezgi left a comment

Choose a reason for hiding this comment

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

you need to register the new toolchains in k8s/k8s.bzl?

@alex1545
Copy link
Contributor Author

/retest

nlopezgi
nlopezgi reviewed Oct 31, 2018
View reviewed changes
Copy link
Contributor

@nlopezgi nlopezgi left a comment

Choose a reason for hiding this comment

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

looks good, you probably want to update the branch as that will re-trigger tests. Once all tests pass I can approve and merge.

@alex1545
Copy link
Contributor Author

alex1545 commented Nov 1, 2018

/retest

1 similar comment
@nlopezgi
Copy link
Contributor

nlopezgi commented Nov 1, 2018

/retest

chrislovecnm
chrislovecnm reviewed Nov 1, 2018
View reviewed changes
Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Do we need docs or tests?

@nlopezgi
Copy link
Contributor

nlopezgi commented Nov 1, 2018

@chrislovecnm testing of toolchains (other than just verifying rules are not broken) is not possible. Documentation will come later once we have a full solution for toolchains in place (i.e., this PR is work in progress towards getting a full k8s toolchain, we decided to split up the work in small PRs to make review simpler)

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nlopezgi

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

@nlopezgi nlopezgi merged commit 2541c5f into bazelbuild:master Nov 1, 2018
@alex1545 alex1545 deleted the kubectl-via-toolchains branch November 2, 2018 13:15
@smukherj1 smukherj1 mentioned this pull request Nov 7, 2018
Closed
@smukherj1 smukherj1 added the new_rule Indicates whether a PR adds a new rule label Dec 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chrislovecnm chrislovecnm chrislovecnm left review comments

@nlopezgi nlopezgi nlopezgi approved these changes

@fejta fejta Awaiting requested review from fejta

@erain erain Awaiting requested review from erain

@xingao267 xingao267 Awaiting requested review from xingao267

Assignees

@nlopezgi nlopezgi

Labels
approved lgtm new_rule Indicates whether a PR adds a new rule size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alex1545 @chrislovecnm @nlopezgi @k8s-ci-robot @erain @smukherj1