/hack: improve shell script in hack #8143

tanjunchen · 2019-12-18T03:40:03Z

/cc @justinsb @mikesplain @rifelpet @robinpercy
/hold

Let's talk about there changes
i have run ./hack/update-header.sh before i commited it.

tanjunchen · 2019-12-18T03:43:09Z

hack/dev-build.sh

@@ -110,9 +110,9 @@ echo ==========
 echo "Deleting cluster ${CLUSTER_NAME}. Elle est finie."

 kops delete cluster \
-  --name $CLUSTER_NAME \


Double quote to prevent globbing and word splitting

your effort is appreciated!
as for the || exit here and likely in other spots as well, maybe we could set -e at the top?

tanjunchen · 2019-12-18T03:44:23Z

hack/make-apimachinery.sh


 cleanup() {
  chmod -R +w "${WORK_DIR}"
  rm -rf "${WORK_DIR}"
 }
 trap cleanup EXIT

-mkdir -p ${WORK_DIR}/go/


Double quote to prevent globbing and word splitting

tanjunchen · 2019-12-18T03:44:57Z

hack/make-gendocs.sh

@@ -18,45 +18,45 @@

 . $(dirname "${BASH_SOURCE}")/common.sh

-WORK_DIR=`mktemp -d`


Use $(...) notation instead of legacy backticked ...

tanjunchen · 2019-12-18T03:45:59Z

hack/verify-spelling.sh

@@ -12,21 +14,24 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.

-#!/bin/bash


The shebang must be on the first line

tanjunchen · 2019-12-18T03:46:51Z

hack/pre-commit.sh

@@ -12,5 +14,4 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.

-#!/bin/sh -eu


The shebang must be on the first line

tanjunchen · 2019-12-18T03:47:20Z

hack/publish-docs.sh

@@ -14,7 +14,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.

-if ! [ -z $DEBUG ]; then


recommend [ -n .. ] instead of ! [ -z .. ]

tanjunchen · 2019-12-18T03:50:07Z

hack/verify-bazel.sh

@@ -14,7 +15,7 @@
 # limitations under the License.

 KOPS_ROOT=$(git rev-parse --show-toplevel)
-cd ${KOPS_ROOT}


in case cd fails.

tanjunchen · 2019-12-18T03:51:18Z

hack/verify-gomod

@@ -25,7 +25,7 @@ changes=$(git status --porcelain || true)
 if [ -n "${changes}" ]; then
  echo "ERROR: go modules are not up to date; please run: go mod tidy"
  echo "changed files:"
-  printf "${changes}\n"


Don't use variables in the printf format string. Use printf "..%s.." "$foo"

tanjunchen · 2019-12-18T03:54:37Z

hack/verify-apimachinery.sh

@@ -23,7 +23,7 @@ export API_OPTIONS="--verify-only"
 if make apimachinery-codegen; then
 	echo "apimachinery is up to date"
 else
-	echo "\n FAIL: - the verify-apimachinery.sh test failed, apimachinery is not up to date"
-	echo "\n FAIL: - please run the command 'make apimachinery'"
+	echo -e "\n FAIL: - the verify-apimachinery.sh test failed, apimachinery is not up to date"


echo "\n FAIL: - the verify-apimachinery.sh test failed, apimachinery is not up to date"

\n FAIL: - the verify-apimachinery.sh test failed, apimachinery is not up to date

--------------->
echo -e "\n FAIL: - the verify-apimachinery.sh test failed, apimachinery is not up to date"

FAIL: - the verify-apimachinery.sh test failed, apimachinery is not up to date

tanjunchen · 2019-12-18T03:55:18Z

/assign @geojaz

geojaz

I don't have any major problems with this and am in favor of cleaning these scripts up and making them more consistent. I provided some suggestions, but didn't point them out each time.

Was there a specific reason to refactor these files right now or were they broken in your use case?

geojaz · 2019-12-18T08:46:07Z

hack/dev-build.sh

@@ -110,9 +110,9 @@ echo ==========
 echo "Deleting cluster ${CLUSTER_NAME}. Elle est finie."

 kops delete cluster \
-  --name $CLUSTER_NAME \


your effort is appreciated!
as for the || exit here and likely in other spots as well, maybe we could set -e at the top?

geojaz · 2019-12-18T08:55:16Z

hack/go_install_from_commit.sh

@@ -1,4 +1,5 @@
 #!/usr/bin/env bash
+
 # Copyright 2017 The Kubernetes Authors.


This is an open question that I've been running into frequently these days, but don't currently have an answer to. When you touch a file, do you update the boilerplate date? i've seen what appears to be new code PRed with a date in the past, and dates that aren't being updated when code is touched. Do we (cncf/k8s org) have a standard for this? random selection of people who may know: @justinsb @thockin @mikesplain @joshbranham @luxas

so we need to talk about it

Yeah I am unsure so curious to hear from others here

geojaz · 2019-12-18T08:56:18Z

hack/make-apimachinery.sh

-mkdir -p ${WORK_DIR}/go/
-ln -s ${GOPATH}/src/k8s.io/kops/vendor/ ${WORK_DIR}/go/src
+mkdir -p "${WORK_DIR}"/go/
+ln -s "${GOPATH}"/src/k8s.io/kops/vendor/ "${WORK_DIR}"/go/src


Suggested change

ln -s "${GOPATH}"/src/k8s.io/kops/vendor/ "${WORK_DIR}"/go/src

ln -s "${GOPATH}/src/k8s.io/kops/vendor/" "${WORK_DIR}/go/src"

geojaz · 2019-12-18T08:56:42Z

hack/make-apimachinery.sh


 unset GOBIN
 GOPATH=${WORK_DIR}/go/ go install -v k8s.io/code-generator/cmd/conversion-gen/
-cp ${WORK_DIR}/go/bin/conversion-gen ${GOPATH}/bin/
+cp "${WORK_DIR}"/go/bin/conversion-gen "${GOPATH}"/bin/


Suggested change

cp "${WORK_DIR}"/go/bin/conversion-gen "${GOPATH}"/bin/

cp "${WORK_DIR}/go/bin/conversion-gen" "${GOPATH}/bin/"

(and so on. i did not point out each incident of this stylistic suggestion)

geojaz · 2019-12-18T08:59:08Z

hack/dev-build.sh

-  --name $CLUSTER_NAME \
-  --state $KOPS_STATE_STORE \
-  -v $VERBOSITY \
+  --name "$CLUSTER_NAME" \


the rest of the interpolations mostly use "${CLUSTER_NAME}" form. if you're going to add the "'s, I think we may as well go the whole way.

tanjunchen · 2019-12-25T09:23:18Z

/cc @geojaz
/cc @joshbranham @justinsb @rifelpet
The so many scripts are changed before , I think it is not a good way.
we can open an new pr to improve the other files in the futrue , it is good for everybody.
/hold cancel

justinsb · 2019-12-28T01:58:37Z

These changes all look good, and we can always do more cleanups in other PRs.

We don't have a position that we know of on updating copyright years.

/approve
/lgtm

k8s-ci-robot · 2019-12-28T01:59:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, tanjunchen

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:

~~OWNERS~~ [justinsb]

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 a review from justinsb December 18, 2019 03:40

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 18, 2019

k8s-ci-robot requested review from mikesplain, rifelpet and robinpercy December 18, 2019 03:40

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 18, 2019

tanjunchen changed the title ~~/hack: improve shell in hack~~ /hack: improve shell script in hack Dec 18, 2019

tanjunchen commented Dec 18, 2019

View reviewed changes

k8s-ci-robot assigned geojaz Dec 18, 2019

geojaz reviewed Dec 18, 2019

View reviewed changes

/hack: improve shell script in hack

cc0f702

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 25, 2019

k8s-ci-robot requested review from geojaz and joshbranham December 25, 2019 09:23

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 25, 2019

k8s-ci-robot assigned justinsb Dec 28, 2019

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

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

k8s-ci-robot merged commit e7ad6a9 into kubernetes:master Dec 28, 2019

k8s-ci-robot added this to the v1.18 milestone Dec 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

/hack: improve shell script in hack #8143

/hack: improve shell script in hack #8143

tanjunchen commented Dec 18, 2019 •

edited

Loading

tanjunchen Dec 18, 2019

geojaz Dec 18, 2019

tanjunchen Dec 18, 2019

tanjunchen Dec 18, 2019

tanjunchen Dec 18, 2019

tanjunchen Dec 18, 2019

tanjunchen Dec 18, 2019

tanjunchen Dec 18, 2019

tanjunchen Dec 18, 2019

tanjunchen Dec 18, 2019

tanjunchen Dec 18, 2019

tanjunchen commented Dec 18, 2019

geojaz left a comment

geojaz Dec 18, 2019

geojaz Dec 18, 2019

tanjunchen Dec 18, 2019

joshbranham Dec 18, 2019

geojaz Dec 18, 2019

geojaz Dec 18, 2019

geojaz Dec 18, 2019

geojaz Dec 18, 2019

tanjunchen commented Dec 25, 2019

justinsb commented Dec 28, 2019

k8s-ci-robot commented Dec 28, 2019

		@@ -18,45 +18,45 @@

		. $(dirname "${BASH_SOURCE}")/common.sh

		WORK_DIR=`mktemp -d`

		@@ -1,4 +1,5 @@
		#!/usr/bin/env bash

		# Copyright 2017 The Kubernetes Authors.

	ln -s "${GOPATH}"/src/k8s.io/kops/vendor/ "${WORK_DIR}"/go/src
	ln -s "${GOPATH}/src/k8s.io/kops/vendor/" "${WORK_DIR}/go/src"

	cp "${WORK_DIR}"/go/bin/conversion-gen "${GOPATH}"/bin/
	cp "${WORK_DIR}/go/bin/conversion-gen" "${GOPATH}/bin/"

/hack: improve shell script in hack #8143

/hack: improve shell script in hack #8143

Conversation

tanjunchen commented Dec 18, 2019 • edited Loading

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

tanjunchen commented Dec 18, 2019

geojaz 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

Choose a reason for hiding this comment

tanjunchen commented Dec 25, 2019

justinsb commented Dec 28, 2019

k8s-ci-robot commented Dec 28, 2019

tanjunchen commented Dec 18, 2019 •

edited

Loading