Fix shellcheck failure in gce/gci/flexvolume_node_setup.sh #81061

k-toyoda-pi · 2019-08-07T04:56:48Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Fix shellcheck failure of cluster/gce/gci/flexvolume_node_setup.sh.

Almost all of detected failures are SC2086: Double quote to prevent globbing and word splitting.
Also, the below failures are detected in the file. See changed lines.

SC2012: Use find instead of ls to better handle non-alphanumeric filenames.
SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.

Which issue(s) this PR fixes:
Ref #72956

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

/sig testing
/priority backlog

k-toyoda-pi · 2019-08-07T04:58:13Z

cluster/gce/gci/flexvolume_node_setup.sh

@@ -175,8 +175,7 @@ echo "Restarting Kubelet..."
 echo

 systemctl restart kubelet.service
-kubelet_wait
-if [ $? -eq 0 ]; then
+if kubelet_wait; then


if [ $? -eq 0 ]; then ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.

k-toyoda-pi · 2019-08-07T04:59:06Z

cluster/gce/gci/flexvolume_node_setup.sh

    if [ -d "$driver_dir" ]; then

-      filecount=$(ls -1 $driver_dir | wc -l)
-      if [ $filecount -gt 1 ]; then
+      filecount=$(find "$driver_dir" -mindepth 1 -maxdepth 1 -printf "%P\n" | grep -c -v "^\.")


filecount=$(ls -1 $driver_dir | wc -l) ^---------------^ SC2012: Use find instead of ls to better handle non-alphanumeric filenames. ^---------^ SC2086: Double quote to prevent globbing and word splitting.

It is not same result with original code even if ls is replaced with find simply.
It uses -printf "%P\n" to output only file name, and use grep -v "^\." to except files starting with ..

After changing, shellsheck says SC2126: Consider using grep -c instead of grep|wc -l.
Therefore, it removes wc -l and adds grep -c.

darwin's version of find doesn't have -printf

the original implementation is way simpler, and since this is used for counting and not the specific paths I think it's acceptable to ignore SC2012 here

if we did actually want the values and not the count, we have an established pattern elsewhere:

use a subshell

cd to the target search directory

use find against .

trim ./ from the beginning by piping to cut or similar

To get filename, use print0 and basename.

+1 on not needing it here

Thank you for the comments.
I modified it a little as follows. As far as I run, the result is equivalent to the original way. But, more complicated and not readable.

filecount=$(cd "$driver_dir"; find . -mindepth 1 -maxdepth 1 -print0 | xargs -0 -n1 basename | grep -c -v "^\.")

Can we ignore SC2012?

Specifically for getting a count, that's probably fine.
But generally speaking SC2012 exists because ls output is NOT portable or specified. We should prefer find.

Longer term we should move elaborate logic out of bash.

I see. I updated the commit using -print0 and basename. Could you check it?

k-toyoda-pi · 2019-08-07T05:00:24Z

cluster/gce/gci/flexvolume_node_setup.sh

        echo "ERROR: Expected 1 file in the FlexVolume directory but found $filecount."
        exit 1
      fi

-      driver_file=$( ls $driver_dir | head -n 1 )
+      driver_file=$(find "$driver_dir" -mindepth 1 -maxdepth 1 -printf "%P\n" | grep -v "^\." | head -n 1)


driver_file=$( ls $driver_dir | head -n 1 ) ^------------^ SC2012: Use find instead of ls to better handle non-alphanumeric filenames. ^---------^ SC2086: Double quote to prevent globbing and word splitting.

Same with line 105.

same here, I think it's acceptable to ignore SC2012

instead of printf, use normal print, and use basename as the last step in the chain

I have done.

k-toyoda-pi · 2019-08-09T07:14:00Z

/test pull-kubernetes-integration

spiffxp

thanks for the explanations

spiffxp · 2019-08-09T18:49:57Z

cluster/gce/gci/flexvolume_node_setup.sh

    if [ -d "$driver_dir" ]; then

-      filecount=$(ls -1 $driver_dir | wc -l)
-      if [ $filecount -gt 1 ]; then
+      filecount=$(find "$driver_dir" -mindepth 1 -maxdepth 1 -printf "%P\n" | grep -c -v "^\.")


darwin's version of find doesn't have -printf

the original implementation is way simpler, and since this is used for counting and not the specific paths I think it's acceptable to ignore SC2012 here

spiffxp · 2019-08-09T18:50:25Z

cluster/gce/gci/flexvolume_node_setup.sh

        echo "ERROR: Expected 1 file in the FlexVolume directory but found $filecount."
        exit 1
      fi

-      driver_file=$( ls $driver_dir | head -n 1 )
+      driver_file=$(find "$driver_dir" -mindepth 1 -maxdepth 1 -printf "%P\n" | grep -v "^\." | head -n 1)


same here, I think it's acceptable to ignore SC2012

BenTheElder · 2019-08-14T23:53:20Z

cluster/gce/gci/flexvolume_node_setup.sh

    if [ -d "$driver_dir" ]; then

-      filecount=$(ls -1 $driver_dir | wc -l)
-      if [ $filecount -gt 1 ]; then
+      filecount=$(cd "$driver_dir"; find . -mindepth 1 -maxdepth 1 -print0 | xargs -0 -n1 basename | grep -c -v "^\.")


we shouldn't need to do grep after the basename?

This line counts files under $driver_dir. Therefore, I think need grep -c to count.

we can count with something like wc

I considered using wc -l, but shellcheck said SC2126.

... xargs -0 -n1 basename | grep -v "^\." | wc -l) ^-----------^ SC2126: Consider using grep -c instead of grep|wc -l.

like I said up top for both of these now-find-based-implementations, I feel like the original ls-based implementations are fine; it's not clear to me what value we're getting here by adding all of this extra complexity to work around corner cases that haven't crept up thus far

@k-toyoda-pi er right, see discussion below for if we even need to ignore files starting with . (I don't think so, however we probably should ignore directories)
@spiffxp discussing below. #81061 (comment)

BenTheElder · 2019-08-14T23:54:10Z

cluster/gce/gci/flexvolume_node_setup.sh

        echo "ERROR: Expected 1 file in the FlexVolume directory but found $filecount."
        exit 1
      fi

-      driver_file=$( ls $driver_dir | head -n 1 )
+      driver_file=$(cd "$driver_dir"; find . -mindepth 1 -maxdepth 1 -print0 | xargs -0 -n1 basename | grep -v "^\." | head -n 1)


we should prefer cut to grep -v since we are removing a known fixed length prefix from all entries.

kubernetes/hack/lib/util.sh

Lines 541 to 544 in 8c6c94b

function kube::util::list_staging_repos() {

(

cd "${KUBE_ROOT}/staging/src/k8s.io" && \

find . -mindepth 1 -maxdepth 1 -type d | cut -c 3- | sort

find reveals files starting with .(ex. .aaa). But ls without -a does not. So, grep -v in this line excepts the files.

I see now, we already pipe through basename which drops the ./foo/, but do we actually need to ignore these?

If find outputs the following result, I think grep -v is needed to exclude .ddd.

$ cd find_test; find . -mindepth 1 -print0 | xargs -0 -n1 basename aaa .ddd bbb ccc

Should we not consider these case?

I'm not sure, I guess it depends on the data. Let's handle it for now but maybe leave a TODO about it.

also cc @msau42 ... does it make sense to ignore drivers starting with . ?

directories that start with . are reserved for the driver installation mechanism (to make sure driver installs are atomic). See here for details

directories sure, files though?

like I said up top for both of these now-find-based-implementations, I feel like the original ls-based implementations are fine; it's not clear to me what value we're getting here by adding all of this extra complexity to work around corner cases that haven't crept up thus far

the ls implementation was already not-correct I suspect. in neither of these are we currently only finding files, despite the variable name.

Thank you for many comments.
I've checked flexvolume behavior.

When I placed kubelet-plugins/volume/exec/test~.aaa/.aaa, which is dummy driver, .aaa can work.
From the result, I think it should not except files begin with ".".

I will modify as the below.

(line:105) filecount=$(cd "$driver_dir"; find . -mindepth 1 -maxdepth 1 -print0 | xargs -0 -n1 basename | wc -l)

(line:111) driver_file=$(cd "$driver_dir"; find . -mindepth 1 -maxdepth 1 -print0 | xargs -0 -n1 basename | head -n 1)

SGTM, thanks for being thorough!

Thanks, I'm done.

BenTheElder · 2019-09-04T01:27:15Z

/retest

BenTheElder · 2019-09-04T01:30:32Z

/lgtm
/approve

k-toyoda-pi · 2019-09-04T04:22:44Z

/test pull-kubernetes-integration

BenTheElder · 2019-09-07T18:20:39Z

/assign @filbranden
(feel free to unassign and or update OWNERS 😉)

filbranden · 2019-09-09T16:33:14Z

/unassign

Good point @BenTheElder ! Will send a PR to update OWNERS...

Thanks!
Filipe

k-toyoda-pi · 2019-09-11T23:53:04Z

/assing @yguo0905
for approval

BenTheElder · 2019-09-16T18:26:25Z

/assign @yguo0905
(corrected the command)

yguo0905 · 2019-09-16T18:38:17Z

cc @verult
/approve

k8s-ci-robot · 2019-09-16T18:38:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, k-toyoda-pi, yguo0905

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:

~~cluster/gce/gci/OWNERS~~ [yguo0905]
~~hack/OWNERS~~ [BenTheElder]

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

k8s-ci-robot requested review from jszczepkowski and spiffxp August 7, 2019 04:57

k-toyoda-pi commented Aug 7, 2019

View reviewed changes

k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Aug 7, 2019

k-toyoda-pi commented Aug 7, 2019

View reviewed changes

k-toyoda-pi mentioned this pull request Aug 7, 2019

Eliminate shellcheck failures #72956

Closed

spiffxp suggested changes Aug 9, 2019

View reviewed changes

BenTheElder reviewed Aug 14, 2019

View reviewed changes

fix shellcheck failure in gci/flexvolume_node_setup.sh

5c724f6

k8s-ci-robot assigned BenTheElder Sep 4, 2019

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

k8s-ci-robot assigned filbranden Sep 7, 2019

k8s-ci-robot unassigned filbranden Sep 9, 2019

k8s-ci-robot assigned yguo0905 Sep 16, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2019

k8s-ci-robot merged commit 1bebaea into kubernetes:master Sep 16, 2019

k8s-ci-robot added this to the v1.17 milestone Sep 16, 2019

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

Fix shellcheck failure in gce/gci/flexvolume_node_setup.sh #81061

Fix shellcheck failure in gce/gci/flexvolume_node_setup.sh #81061

Conversation

k-toyoda-pi commented Aug 7, 2019

Choose a reason for hiding this comment

k-toyoda-pi Aug 7, 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

k-toyoda-pi Aug 7, 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

k-toyoda-pi commented Aug 9, 2019

spiffxp left a 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

Choose a reason for hiding this comment

BenTheElder Aug 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Sep 4, 2019

BenTheElder commented Sep 4, 2019

k-toyoda-pi commented Sep 4, 2019

BenTheElder commented Sep 7, 2019

filbranden commented Sep 9, 2019

k-toyoda-pi commented Sep 11, 2019

BenTheElder commented Sep 16, 2019

yguo0905 commented Sep 16, 2019

k8s-ci-robot commented Sep 16, 2019

k-toyoda-pi Aug 7, 2019 •

edited

k-toyoda-pi Aug 7, 2019 •

edited

BenTheElder Aug 17, 2019 •

edited