Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix shellcheck failures of hack/update-generated-kms-dockerized.sh and hack/update-generated-protobuf-dockerized.sh #76478

Merged
merged 1 commit into from Apr 17, 2019

Conversation

@SataQiu
Copy link
Member

SataQiu commented Apr 12, 2019

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
Fix shellcheck failures of

  • hack/update-generated-kms-dockerized.sh
  • hack/update-generated-protobuf-dockerized.sh

Which issue(s) this PR fixes:

Ref #72956

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

/priority important-soon

@SataQiu

This comment has been minimized.

Copy link
Member Author

SataQiu commented Apr 12, 2019

/sig release

@k8s-ci-robot k8s-ci-robot added sig/release and removed needs-sig labels Apr 12, 2019

@k8s-ci-robot k8s-ci-robot requested review from eparis and ixdy Apr 12, 2019

@SataQiu SataQiu force-pushed the SataQiu:fix-shell-hack-20190412 branch from e90a973 to 557b2e2 Apr 12, 2019


# Update boilerplate for the generated file.
echo "$(cat hack/boilerplate/boilerplate.generatego.txt ${KUBE_KMS_GRPC_ROOT}/service.pb.go)" > ${KUBE_KMS_GRPC_ROOT}/service.pb.go
cat hack/boilerplate/boilerplate.generatego.txt "${KUBE_KMS_GRPC_ROOT}/service.pb.go" > tmpfile && mv tmpfile "${KUBE_KMS_GRPC_ROOT}/service.pb.go"

This comment has been minimized.

Copy link
@SataQiu

SataQiu Apr 12, 2019

Author Member

change reason:

In hack/update-generated-kms-dockerized.sh line 57:
echo "$(cat hack/boilerplate/boilerplate.generatego.txt ${KUBE_KMS_GRPC_ROOT}/service.pb.go)" > ${KUBE_KMS_GRPC_ROOT}/service.pb.go
     ^-- SC2005: Useless echo? Instead of 'echo $(cmd)', just use 'cmd'.
                                                        ^-- SC2086: Double quote to prevent globbing and word splitting.
                                                                                                ^-- SC2086: Double quote to prevent globbing and word splitting.
In hack/update-generated-kms-dockerized.sh line 57:
cat hack/boilerplate/boilerplate.generatego.txt "${KUBE_KMS_GRPC_ROOT}/service.pb.go" > "${KUBE_KMS_GRPC_ROOT}/service.pb.go"
                                                ^-- SC2094: Make sure not to read and write the same file in the same pipeline.
                                                                                        ^-- SC2094: Make sure not to read and write the same file in the same pipeline.

Ref: https://github.com/koalaman/shellcheck/wiki/SC2094

This comment has been minimized.

Copy link
@cblecker

cblecker Apr 14, 2019

Member

Two concerns with this.. first echo by default appends a \n to the end of the file. Can we verify the new pattern does this too?

Additionally, I don't want the tmp file to be left behind if this command gets interrupted. Can we use something like "${KUBE_KMS_GRPC_ROOT}/service.pb.go.tmp" and add that filename to the cleanup function on line 42?

This comment has been minimized.

Copy link
@SataQiu

SataQiu Apr 14, 2019

Author Member

Thanks for your feedback. @cblecker
I have verified the new pattern.
The contents of the files generated by these two methods are the same.
图片

@@ -41,7 +41,7 @@ fi

gotoprotobuf=$(kube::util::find-binary "go-to-protobuf")

APIROOTS=( ${1} )
mapfile -t APIROOTS <<< "${1}"

This comment has been minimized.

Copy link
@SataQiu

SataQiu Apr 12, 2019

Author Member

change reason:

In ./hack/update-generated-protobuf-dockerized.sh line 44:
APIROOTS=( ${1} )
           ^--^ SC2206: Quote to prevent word splitting, or split robustly with mapfile or read -a.

Ref: https://github.com/koalaman/shellcheck/wiki/SC2206

This comment has been minimized.

Copy link
@cblecker

cblecker Apr 14, 2019

Member

We can't use mapfile as it's not compatible with Bash v3. Prefer the read -a pattern. There are a number of examples of read-a in the scripts.

This comment has been minimized.

Copy link
@SataQiu

SataQiu Apr 14, 2019

Author Member

Done!

@SataQiu

This comment has been minimized.

Copy link
Member Author

SataQiu commented Apr 12, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

1 similar comment
@xichengliudui

This comment has been minimized.

Copy link
Contributor

xichengliudui commented Apr 12, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@SataQiu SataQiu force-pushed the SataQiu:fix-shell-hack-20190412 branch from 557b2e2 to 0200e32 Apr 14, 2019

@SataQiu

This comment has been minimized.

Copy link
Member Author

SataQiu commented Apr 14, 2019

/assign @fejta @cblecker

@@ -41,7 +41,7 @@ fi

gotoprotobuf=$(kube::util::find-binary "go-to-protobuf")

APIROOTS=( ${1} )
mapfile -t APIROOTS <<< "${1}"

This comment has been minimized.

Copy link
@cblecker

cblecker Apr 14, 2019

Member

We can't use mapfile as it's not compatible with Bash v3. Prefer the read -a pattern. There are a number of examples of read-a in the scripts.


# Update boilerplate for the generated file.
echo "$(cat hack/boilerplate/boilerplate.generatego.txt ${KUBE_KMS_GRPC_ROOT}/service.pb.go)" > ${KUBE_KMS_GRPC_ROOT}/service.pb.go
cat hack/boilerplate/boilerplate.generatego.txt "${KUBE_KMS_GRPC_ROOT}/service.pb.go" > tmpfile && mv tmpfile "${KUBE_KMS_GRPC_ROOT}/service.pb.go"

This comment has been minimized.

Copy link
@cblecker

cblecker Apr 14, 2019

Member

Two concerns with this.. first echo by default appends a \n to the end of the file. Can we verify the new pattern does this too?

Additionally, I don't want the tmp file to be left behind if this command gets interrupted. Can we use something like "${KUBE_KMS_GRPC_ROOT}/service.pb.go.tmp" and add that filename to the cleanup function on line 42?

@SataQiu SataQiu force-pushed the SataQiu:fix-shell-hack-20190412 branch 2 times, most recently from f509f21 to db8bc68 Apr 14, 2019

@SataQiu

This comment has been minimized.

Copy link
Member Author

SataQiu commented Apr 14, 2019

/test pull-kubernetes-integration

fix shellcheck failures of hack/update-generated-kms-dockerized.sh ha…
…ck/update-generated-protobuf-dockerized.sh
@SataQiu

This comment has been minimized.

Copy link
Member Author

SataQiu commented Apr 15, 2019

@cblecker I'm sorry to bother you, but could you review it again?

@SataQiu

This comment has been minimized.

Copy link
Member Author

SataQiu commented Apr 16, 2019

Can you review this PR again? @cblecker

@k-toyoda-pi

This comment has been minimized.

Copy link
Contributor

k-toyoda-pi commented Apr 17, 2019

Hi, I checked your patch and ran with it. I think your patch is appropriate.

@SataQiu

This comment has been minimized.

Copy link
Member Author

SataQiu commented Apr 17, 2019

/cc @fejta
Can you take a look at this PR?

@k8s-ci-robot k8s-ci-robot requested a review from fejta Apr 17, 2019

@fejta
Copy link
Contributor

fejta left a comment

/lgtm
/approve


# Update boilerplate for the generated file.
echo "$(cat hack/boilerplate/boilerplate.generatego.txt ${KUBE_KMS_GRPC_ROOT}/service.pb.go)" > ${KUBE_KMS_GRPC_ROOT}/service.pb.go
cat hack/boilerplate/boilerplate.generatego.txt "${KUBE_KMS_GRPC_ROOT}/service.pb.go" > "${KUBE_KMS_GRPC_ROOT}/service.pb.go.tmp" && \

This comment has been minimized.

Copy link
@fejta

fejta Apr 17, 2019

Contributor

I would separate these into two lines, no need to use &&

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 17, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-ci-robot k8s-ci-robot merged commit f625bef into kubernetes:master Apr 17, 2019

20 checks passed

cla/linuxfoundation SataQiu authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.