fix vendor scripts shellcheck failures #79316

BenTheElder · 2019-06-23T23:11:04Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it: fixes shellcheck failures in hack/update-vendor.sh, hack/pin-dependency.sh, and hack/verify-no-vendor-cycles.sh.

Which issue(s) this PR fixes:

Part of #72956

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

BenTheElder · 2019-06-23T23:12:57Z

hack/update-vendor.sh

@@ -47,6 +47,15 @@ if [ -z "${BASH_XTRACEFD:-}" ]; then
  set -x
 fi

+# list_staging_repos outputs a sorted list of repos in staging/src/k8s.io
+# each entry will just be the $repo portion of staging/src/k8s.io/$repo/...
+function list_staging_repos() {


Correctly listing these portably and sorted in one place had a nice side effect of simplifying the script a bit.
I implemented a couple variants of this, but this is probably the most robust version.

moved to kube::util as I found the other vendor scripts need to do the same thing a lot..

BenTheElder · 2019-06-23T23:13:34Z

hack/update-vendor.sh

@@ -67,17 +76,17 @@ function ensure_require_replace_directives_for_all_dependencies() {
  go mod edit -json | jq -r ".Replace // [] | sort | .[] | select(${replace_filter})" > "${replace_json}"

  # 1a. Ensure replace directives have an explicit require directive
-  cat "${replace_json}" | jq -r '"-require \(.Old.Path)@\(.New.Version)"'             | xargs -L 100 go mod edit -fmt
+  jq -r '"-require \(.Old.Path)@\(.New.Version)"' < "${replace_json}" | xargs -L 100 go mod edit -fmt


unnecessary cat usage. we can just pipe the file to input. all of the changes on .*cat.* lines are this.

BenTheElder · 2019-06-23T23:14:32Z

hack/update-vendor.sh

+    echo "replace (";
+    cat "${go_mod_replace}";
+    echo ")";
+  } > go.mod


Here we've used a command group to redirect once, which is simpler (and preferred by shellcheck).

It has a super minor performance benefit of only needing to open the file once since bash know understands the intent to redirect all of these to the same output.

What is the difference between { foo } and ( foo )?

{ foo } is a command group but does not involve a subshell. ( foo ) is a command group in a subshell. using { foo; } or:

{ foo }

gets the benefits of grouping without involving a subprocess / shell.

technically there are also some slightly different rules for parsing to identify them that don't apply here.

BenTheElder · 2019-06-23T23:15:19Z

hack/update-vendor.sh

@@ -201,9 +214,15 @@ done
 kube::log::status "go.mod: sorting staging modules"
 # tidy staging repos in reverse dependency order.
 # the content of dependencies' go.mod files affects what `go mod tidy` chooses to record in a go.mod file.
-ls staging/src/k8s.io | sort | xargs -I {} echo "k8s.io/{}" > "${TMP_DIR}/tidy_unordered.txt"
+tidy_unordered="${TMP_DIR}/tidy_unordered.txt"


not strictly necessary, but we were computing this in at least 3 places, so >WET ==> DRY

BenTheElder · 2019-06-23T23:16:14Z

hack/update-vendor.sh

+# We do read from ${tidy_unordered} in the pipeline and mention it within the
+# pipeline (but only ready it again) so we disable the lint to assure shellcheck
+# that :this-is-fine:
+# shellcheck disable=SC2094


I dislike needing these disable statements, but in this case it's a warning about potentially reading and writing to the same path in a loop (which wouldn't be safe). Since shellcheck has to just guess based on where the path is being referenced that it might be hitting this, we have to confirm that it is not.

BenTheElder · 2019-06-23T23:16:59Z

/sig release
/priority important-longterm

BenTheElder · 2019-06-23T23:38:36Z

hack/lib/util.sh

@@ -531,6 +531,16 @@ EOF
 EOF
 }

+# list_staging_repos outputs a sorted list of repos in staging/src/k8s.io
+# each entry will just be the $repo portion of staging/src/k8s.io/$repo/...
+function kube::util::list_staging_repos() {


originally I didn't add this to util, but all 3 fixed scripts have a need for this and doing it correctly / portably is tricky

BenTheElder · 2019-06-24T00:02:21Z

/retest

BenTheElder · 2019-06-24T00:24:21Z

hack/lib/util.sh

+  (
+    cd "${KUBE_ROOT}/staging/src/k8s.io" && \
+    find . -type d -mindepth 1 -maxdepth 1 | cut -c 3- | sort
+  )


explanation:

ls output is not portable

find is more portable but gives relative paths

-> cd to the search directory in a subshell, then trim off the leading ./ from results.
search only for directories, set min/max depth so we only get results immediately below the search dir.

fejta

/lgtm
/approve

fejta · 2019-06-24T21:48:00Z

hack/update-vendor.sh

+    echo "replace (";
+    cat "${go_mod_replace}";
+    echo ")";
+  } > go.mod


What is the difference between { foo } and ( foo )?

k8s-ci-robot · 2019-06-24T21:48:45Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~hack/OWNERS~~ [BenTheElder,fejta]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta · 2019-06-24T21:57:59Z

/retest

fejta-bot · 2019-06-25T03:39:14Z

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

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

BenTheElder · 2019-06-25T05:55:59Z

#78913 grrr
/retest

k8s-ci-robot requested review from fejta and sttts June 23, 2019 23:12

BenTheElder commented Jun 23, 2019

View reviewed changes

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 23, 2019

BenTheElder force-pushed the shellll branch from f059ce3 to 7f7ecd6 Compare June 23, 2019 23:36

BenTheElder changed the title ~~fix hack/update-vendor.sh shellcheck failures~~ fix vendor scripts shellcheck failures Jun 23, 2019

BenTheElder commented Jun 23, 2019

View reviewed changes

BenTheElder force-pushed the shellll branch 2 times, most recently from 6976ec1 to 95876cb Compare June 23, 2019 23:45

BenTheElder mentioned this pull request Jun 24, 2019

fix hack/make-rules/make-help.sh shellcheck failures #79318

Merged

BenTheElder force-pushed the shellll branch from 95876cb to 0c9edb4 Compare June 24, 2019 00:23

BenTheElder commented Jun 24, 2019

View reviewed changes

BenTheElder added 5 commits June 24, 2019 11:43

add kube::util::list_staging_repos

29b7b2c

fix hack/update-vendor.sh shellcheck failures

47daf19

fix hack/pin-dependency.sh shellcheck failures

1741d93

fix hack/verify-no-vendor-cycles.sh shellcheck failures

7e96be3

document KUBE_ROOT in util.sh

4aaa7b4

BenTheElder force-pushed the shellll branch from 0c9edb4 to 4aaa7b4 Compare June 24, 2019 18:43

fejta reviewed Jun 24, 2019

View reviewed changes

k8s-ci-robot assigned fejta Jun 24, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2019

k8s-ci-robot merged commit dac0bd9 into kubernetes:master Jun 25, 2019

BenTheElder deleted the shellll branch June 25, 2019 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix vendor scripts shellcheck failures #79316

fix vendor scripts shellcheck failures #79316

BenTheElder commented Jun 23, 2019 •

edited

BenTheElder Jun 23, 2019

BenTheElder Jun 23, 2019

BenTheElder Jun 23, 2019

BenTheElder Jun 23, 2019

fejta Jun 24, 2019

BenTheElder Jun 24, 2019

BenTheElder Jun 23, 2019

BenTheElder Jun 23, 2019

BenTheElder commented Jun 23, 2019

BenTheElder Jun 23, 2019

BenTheElder commented Jun 24, 2019

BenTheElder Jun 24, 2019

fejta left a comment

fejta Jun 24, 2019

k8s-ci-robot commented Jun 24, 2019

fejta commented Jun 24, 2019

fejta-bot commented Jun 25, 2019

BenTheElder commented Jun 25, 2019

fix vendor scripts shellcheck failures #79316

fix vendor scripts shellcheck failures #79316

Conversation

BenTheElder commented Jun 23, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Jun 23, 2019

Choose a reason for hiding this comment

BenTheElder commented Jun 24, 2019

Choose a reason for hiding this comment

fejta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 24, 2019

fejta commented Jun 24, 2019

fejta-bot commented Jun 25, 2019

BenTheElder commented Jun 25, 2019

BenTheElder commented Jun 23, 2019 •

edited