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

kubectl via toolchains (continued) #212

Merged
merged 8 commits into from
Nov 9, 2018

Conversation

alex1545
Copy link
Contributor

@alex1545 alex1545 commented Nov 2, 2018

WORKSPACE rule to configure the kubectl tool. For now it onle autodetects the cubectl path.

k8s/k8s.bzl Outdated Show resolved Hide resolved
k8s/kubectl_configure.bzl Outdated Show resolved Hide resolved
toolchains/kubectl/BUILD Outdated Show resolved Hide resolved
toolchains/kubectl/BUILD Outdated Show resolved Hide resolved
@fejta
Copy link
Contributor

fejta commented Nov 2, 2018

/uncc
LGTM to general concept, Nicolas seems better informed about the starlark than me 😁

@k8s-ci-robot k8s-ci-robot removed the request for review from fejta November 2, 2018 20:14
@alex1545
Copy link
Contributor Author

alex1545 commented Nov 3, 2018

/retest

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.

We need documentation and tests please

@alex1545
Copy link
Contributor Author

alex1545 commented Nov 5, 2018

/retest

k8s/k8s.bzl Outdated Show resolved Hide resolved
k8s/kubectl_configure.bzl Outdated Show resolved Hide resolved
k8s/kubectl_configure.bzl Outdated Show resolved Hide resolved
toolchains/kubectl/BUILD Outdated Show resolved Hide resolved
…oolchain target. Undoing deletion of the static kubectl toolchain rule for internal use.
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.

minor nits, thanks for getting the template thing to work

@@ -0,0 +1,31 @@
# Copyright 2018 The Bazel Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

lets move this file to toolchains/kubectl so the reference to :BUILD.tpl can be relative

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved kubectl_configure.bzl to toolchains/kubectl.
But the reference to BUILD.tpl still has to be absolute. If (in repository_ctx.template) a string or a path object is used to specify the template file, then it will be relative to the local_k8s_config repo. If a Label is used, then an absolute path must be specified.

k8s/kubectl_configure.bzl Outdated Show resolved Hide resolved
toolchains/kubectl/BUILD.tpl Show resolved Hide resolved
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.

can you also please sync with suvanjan about #215 to see how to address that (does not need to be in this PR, but make sure you dont create conflicts between the work to fix that issue and this work)

toolchains/kubectl/kubectl_configure.bzl Outdated Show resolved Hide resolved
@alex1545
Copy link
Contributor Author

alex1545 commented Nov 8, 2018

From my understanding, the fix to #215 will just remove the exec_compatible_with attribute from the three toolchain targets that are in //toolchains/kubectl/BUILD.

I did this change locally and run the e2e test successfully. But will definitely look into the PR that will follow.

@pierreis
Copy link
Contributor

pierreis commented Nov 8, 2018

FYI, I did just submit the PR #218 that removes the exec_compatible_with constraints, per #215

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alex1545, 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

@alex1545
Copy link
Contributor Author

alex1545 commented Nov 8, 2018

/retest

@alex1545
Copy link
Contributor Author

alex1545 commented Nov 8, 2018

Btw, this is a chance to test PR #208 that attempted to solve issue #173 (running e2e tests from different PRs).
PR #218 should run e2e tests now (in parallel with this PR).

@smukherj1
Copy link
Contributor

Tests don't actually run in parallel yet because the prow job parallelism limit is still set to 1

@smukherj1
Copy link
Contributor

Opened #219 to disable the flakiest test

@k8s-ci-robot k8s-ci-robot removed the lgtm label Nov 9, 2018
@k8s-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@nlopezgi nlopezgi merged commit b354bb9 into bazelbuild:master Nov 9, 2018
@alex1545 alex1545 deleted the kubectl-via-toolchains branch November 13, 2018 19:50
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.

None yet

7 participants