-
Notifications
You must be signed in to change notification settings - Fork 135
Conversation
/assign @nlopezgi |
/assign xingao267 |
/ok-to-test |
/joke |
@smukherj1: Never take advice from electrons. They are always negative. In response to this:
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. |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, minor nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once there is an easy way to build kubectl from source (without checking out the entire kubectl repo), would you consider using this rather than the one on PATH?
Aka check out k8s.io/kubectl and build it (not yet supported though unfortunately) https://github.com/kubernetes/kubectl
/retest |
@fejta |
Very cool, there have been some recent requests about this (how to pull kubectl into the build) in the kubernetes.slack.com #bazel channel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting some changes from lessons learnt while I was working on bazelbuild/rules_docker#559
WORKSPACE
Outdated
|
||
# Register the default kubectl toolchain that expects the 'kubectl' | ||
# executable to be in the PATH | ||
register_toolchains( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to the k8s_repositories macro in k8s/k8s.bzl so that users of the k8s repo automatically get this toolchain registered for them. See bazelbuild/rules_docker#559
tools/BUILD
Outdated
load(":kubectl_toolchain.bzl", "kubectl_toolchain") | ||
|
||
# kubectl toolchain type | ||
toolchain_type(name = "toolchain_type_kubectl") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename this to "toolchain_type" instead of "toolchain_type_kubectl". See bazelbuild/rules_docker#559
The recommended flow is to use the label of the package to figure out the specific type of the toolchain. Recommend creating a "//toolchains/kubectl" directory and this entire file and kubectl_toolchain.bzl there. See my PR for reference
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: erain, nlopezgi, smukherj1 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 |
/retest |
1 similar comment
/retest |
Creating a toolchain rule with a single attribute that is a path to the kubectl tool