Add some initial shell parsing tests. #51649

mml · 2017-08-30T19:35:17Z

These just test to see if there is a bash syntax error in these shell
libraries.

NONE

mml · 2017-08-30T19:36:03Z

I have absolutely no idea if this is a good approach or not, so please feel free to suggest alternate/better ways to achieve this.

/assign @ixdy

cblecker · 2017-08-30T20:10:24Z

you probably want something like this to ensure fail on non-zero exit:

set -o errexit
set -o nounset
set -o pipefail

ixdy

basically LG, with a few minor nits/comments.

ixdy · 2017-08-30T21:15:26Z

cluster/common_test.sh

+set -o pipefail
+
+# Setup test environment
+source hack/test-shlib.sh


you might want to base these off ${KUBE_ROOT} so that you can call this script from somewhere besides ${KUBE_ROOT}?

ixdy · 2017-08-30T21:17:37Z

cluster/BUILD

@@ -46,3 +46,33 @@ pkg_tar(
        "//cluster/saltbase:salt-manifests",
    ],
 )
+
+sh_test(
+    name = "common",


may want to suffix all of these with _test. otherwise you have a weird target named //cluster:common which is actually a test, for example.

ixdy · 2017-08-30T21:22:56Z

hack/test-shlib.sh

+set -o pipefail
+
+# HOME is, by default, not set.  Give it a bogus value.
+HOME=${PWD}


maybe use a tmp dir instead?

ixdy · 2017-08-30T21:24:00Z

hack/BUILD

@@ -14,6 +14,11 @@ filegroup(
    visibility = ["//visibility:private"],
 )

+sh_library(


add testonly = True?

ixdy · 2017-08-30T21:25:42Z

cluster/BUILD

+    name = "common",
+    srcs = ["common_test.sh"],
+    data = [
+        "all-srcs",


":all-srcs" (add the leading :) is preferred for referencing rules in the same package.

ixdy · 2017-08-30T21:36:26Z

an alternative to having hack/test-shlib.sh which sets $HOME to something is to set $HOME in the bazel args with --test_env, something like

--- a/build/root/.bazelrc
+++ b/build/root/.bazelrc
@@ -11,3 +11,6 @@ build --sandbox_tmpfs_path=/tmp
 # Ensure that Bazel never runs as root, which can cause unit tests to fail.
 # This flag requires Bazel 0.5.0+
 build --sandbox_fake_username
+
+# Set HOME in the test environment to something bogus
+test --test_env HOME=/tmp/bazel-fake-home

Another alternative is having cluster/common.sh not blow up if $HOME is unset.

I'm not sure whether either of these is better than what you're already doing.

mml · 2017-08-30T21:59:36Z

I don't like the bazel flag option, because it requires that flag or the test signals a failure. It also seems a little too broad for me to make it (because it changes the environment on all tests), so I'd rather not.

The other idea, fixing cluster/common.sh, is easy enough. I'll just use ${HOME:-.}

mml · 2017-08-30T22:09:31Z

@ixdy I think I got all the style fixes and sidestepped most of the other issues by just executing the files directly as tests.

PTAL

ixdy · 2017-08-30T22:32:12Z

cluster/BUILD

+    srcs = ["common.sh"],
+    data = [
+        ":all-srcs",
+        "//hack/lib:all-srcs",


we actually have a sh_library in hack/lib, so //hack/lib is probably a more correct dep here.

ixdy · 2017-08-30T22:32:28Z

lgtm other than small suggestion (x3)

mml · 2017-08-30T22:38:07Z

Oh, wait. Did you mean //hack/lib:lib or just //hack/lib? I'm not clear what the latter would do, but I put the former.

ixdy · 2017-08-30T22:38:42Z

//hack/lib and //hack/lib:lib are basically equivalent - the former implies the latter. either is fine.

maybe squash your commits?

ixdy · 2017-08-30T22:40:40Z

actually, that's false. buildifier (i.e. update-bazel.sh) will simplify //hack/lib:lib to //hack/lib, so you'll need to change that for the verification tests to pass.

ixdy · 2017-08-30T22:47:47Z

/lgtm

roberthbailey · 2017-08-31T05:08:06Z

/approve

k8s-github-robot · 2017-08-31T05:08:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ixdy, mml, roberthbailey

Associated issue: 51642

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cluster/OWNERS~~ [roberthbailey]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

cblecker · 2017-08-31T05:09:37Z

/test all

fejta-bot · 2017-08-31T08:21:52Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-08-31T12:54:51Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

mml · 2017-08-31T15:26:43Z

/retest #51692

mml · 2017-08-31T19:01:47Z

/retest

These just test to see if there is a bash syntax error in these shell libraries. For kubernetes#51642

k8s-github-robot · 2017-09-01T08:12:43Z

Automatic merge from submit-queue (batch tested with PRs 49971, 51357, 51616, 51649, 51372)

Cleanup after kubernetes#51649 Bug: kubernetes#51642

@ixdy

Automatic merge from submit-queue (batch tested with PRs 50579, 50875, 51797, 51807, 51803) Depend on //cluster/lib instead of :all-srcs. Cleanup after kubernetes#51649 Bug: kubernetes#51642 ```release-note NONE ``` /assign @ixdy /assign @roberthbailey

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 30, 2017

k8s-github-robot assigned eparis and roberthbailey Aug 30, 2017

k8s-github-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 30, 2017

k8s-ci-robot assigned ixdy Aug 30, 2017

mml unassigned eparis Aug 30, 2017

mml force-pushed the sh-test branch from 7929ede to f20b794 Compare August 30, 2017 19:38

mml changed the title ~~Add some initial parsing shell tests.~~ Add some initial shell parsing tests. Aug 30, 2017

mml force-pushed the sh-test branch 3 times, most recently from 4e4c2d7 to ef03933 Compare August 30, 2017 21:17

ixdy reviewed Aug 30, 2017

View reviewed changes

k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 30, 2017

mml force-pushed the sh-test branch from 0118357 to f84519d Compare August 30, 2017 22:08

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 30, 2017

mml force-pushed the sh-test branch from f84519d to 1f13de0 Compare August 30, 2017 22:16

ixdy reviewed Aug 30, 2017

View reviewed changes

mml force-pushed the sh-test branch from 1f13de0 to de30351 Compare August 30, 2017 22:35

mml force-pushed the sh-test branch from de30351 to b4bb642 Compare August 30, 2017 22:41

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2017

mml mentioned this pull request Aug 30, 2017

Don't suggest a reviewer I've already assigned. kubernetes/test-infra#4272

Closed

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2017

mml added 2 commits August 31, 2017 14:32

Make cluster/common.sh work even if ${HOME} is unset.

9a21343

Add some initial shell parsing tests.

0271687

These just test to see if there is a bash syntax error in these shell libraries. For kubernetes#51642

mml force-pushed the sh-test branch from b4bb642 to 0271687 Compare August 31, 2017 21:32

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2017

mml added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2017

k8s-github-robot merged commit 28d0077 into kubernetes:master Sep 1, 2017

luxas mentioned this pull request Sep 1, 2017

Simple shell tests could save a lot of contributor time #51642

Closed

mml added a commit to mml/kubernetes that referenced this pull request Sep 1, 2017

Depend on //cluster/lib instead of :all-srcs.

b7ecd72

Cleanup after kubernetes#51649 Bug: kubernetes#51642

This was referenced Sep 1, 2017

Depend on //cluster/lib instead of :all-srcs. #51807

Merged

PR Dashboard: Outgoing shows merged PRs kubernetes/test-infra#4318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some initial shell parsing tests. #51649

Add some initial shell parsing tests. #51649

mml commented Aug 30, 2017

mml commented Aug 30, 2017

cblecker commented Aug 30, 2017

ixdy left a comment

ixdy Aug 30, 2017

ixdy Aug 30, 2017

ixdy Aug 30, 2017

ixdy Aug 30, 2017

ixdy Aug 30, 2017

ixdy commented Aug 30, 2017

mml commented Aug 30, 2017

mml commented Aug 30, 2017

ixdy Aug 30, 2017

mml Aug 30, 2017

ixdy commented Aug 30, 2017

mml commented Aug 30, 2017

ixdy commented Aug 30, 2017

ixdy commented Aug 30, 2017

ixdy commented Aug 30, 2017

roberthbailey commented Aug 31, 2017

k8s-github-robot commented Aug 31, 2017

cblecker commented Aug 31, 2017

fejta-bot commented Aug 31, 2017

fejta-bot commented Aug 31, 2017

mml commented Aug 31, 2017

mml commented Aug 31, 2017

k8s-github-robot commented Sep 1, 2017

Add some initial shell parsing tests. #51649

Add some initial shell parsing tests. #51649

Conversation

mml commented Aug 30, 2017

mml commented Aug 30, 2017

cblecker commented Aug 30, 2017

ixdy left a comment

Choose a reason for hiding this comment

ixdy Aug 30, 2017

Choose a reason for hiding this comment

ixdy Aug 30, 2017

Choose a reason for hiding this comment

ixdy Aug 30, 2017

Choose a reason for hiding this comment

ixdy Aug 30, 2017

Choose a reason for hiding this comment

ixdy Aug 30, 2017

Choose a reason for hiding this comment

ixdy commented Aug 30, 2017

mml commented Aug 30, 2017

mml commented Aug 30, 2017

ixdy Aug 30, 2017

Choose a reason for hiding this comment

mml Aug 30, 2017

Choose a reason for hiding this comment

ixdy commented Aug 30, 2017

mml commented Aug 30, 2017

ixdy commented Aug 30, 2017

ixdy commented Aug 30, 2017

ixdy commented Aug 30, 2017

roberthbailey commented Aug 31, 2017

k8s-github-robot commented Aug 31, 2017

cblecker commented Aug 31, 2017

fejta-bot commented Aug 31, 2017

fejta-bot commented Aug 31, 2017

mml commented Aug 31, 2017

mml commented Aug 31, 2017

k8s-github-robot commented Sep 1, 2017