Refactor: improve shell scripts #3843
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: namejsjeongkr The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
|
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @namejsjeongkr! |
|
Hi @namejsjeongkr. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
namejsjeongkr
force-pushed
the
improve_shell_scripts
branch
from
649de8c to
ce1710d
Compare
| set -o errexit -o nounset -o xtrace | ||
|
|
||
| # Settings: |
There was a problem hiding this comment.
I don't think all of the comments should be removed.
There was a problem hiding this comment.
I would prefer to keep usage documentation inline with the rest of the kubernetes project's scripts.
| rm -rf "${TMP_DIR:?}" | ||
| fi | ||
| CLEANED_UP=true | ||
| if [ "${CLEANED_UP:-false}" = "true" ]; then |
There was a problem hiding this comment.
There are a lot of unnecessary whitespace changes included that make this difficult to review.
There was a problem hiding this comment.
+1, and this script is used widely throughout CI so I'm not keen on sweeping changes without carefully reviewing.
There was a problem hiding this comment.
Thanks, but this is hard to review with the large amount of unnecessary changes.
e2e-k8s.sh
Enhanced error handling by adding set -o pipefail.
Didn't happen? Also this script is written in POSIX shell (not bash, unlike most of thme) for wide-usage, which doesn't have pipefail. (note the shebang)
Improved code structure by extracting repeated tasks into functions.
... this is debatable, a lot of straightforward inline context was removed.
Introduced an upload_file function to reduce code duplication.
Invoking a command with a single flag and two call-site-specific arguments is hardly code duplication?
Implemented use of gsutil -m cp for parallel uploads to improve performance.
I'm doubtful that this is faster for these small files being uploaded one at a time, do we have evidence?
Anyhow we're not having a performance issue here, this script runs in automation and runtime is dominated by cloning and compiling. https://prow.k8s.io/job-history/gs/kubernetes-ci-logs/logs/post-kind-push-binaries
push-latest-cli.sh
Updated error handling to use set -o errexit -o nounset -o pipefail.
Huh? we already were?
Standardized variable declarations using double quotes.
Double quotes should only be used when expansion is being performed. Single quotes prevents accidental expansion (or unquoted when forming an array).
| hack/release/build/cross.sh | ||
|
|
||
| # upload to the bucket | ||
| echo "Uploading to bucket: ${BUCKET}" |
There was a problem hiding this comment.
logging should go to stderr, not stdout. also, I don't think we need all of this logging, it's a quick process, these are small files.
| rm -rf "${TMP_DIR:?}" | ||
| fi | ||
| CLEANED_UP=true | ||
| if [ "${CLEANED_UP:-false}" = "true" ]; then |
There was a problem hiding this comment.
+1, and this script is used widely throughout CI so I'm not keen on sweeping changes without carefully reviewing.
| set -o errexit -o nounset -o xtrace | ||
|
|
||
| # Settings: |
There was a problem hiding this comment.
I would prefer to keep usage documentation inline with the rest of the kubernetes project's scripts.
| PARALLEL="${PARALLEL:-false}" | ||
|
|
||
| log() { | ||
| echo "[$(date +'%Y-%m-%d %H:%M:%S')] $*" |
There was a problem hiding this comment.
I'm not sure if we want to do this, and this is a behavioral change, not a refactor
| @@ -14,18 +14,25 @@ | |||
| # limitations under the License. | |||
|
|
|||
| set -o errexit -o nounset -o pipefail | |||
| set -x; | |||
| set -x | |||
There was a problem hiding this comment.
this has no meaningful difference but /shrug
| upload_file() { | ||
| local file="$1" | ||
| local version="$2" | ||
| gsutil -m cp -P "${file}" "gs://${BUCKET}/${version}/$(basename "${file}")" |
There was a problem hiding this comment.
-m doesn't make sense for small, single file interactions?
And otherwise this was just gsutil cp -P <call site specific arguments> which is clearer inline, similar to cp -R ...
This seems over-abstracted
| @@ -34,36 +41,42 @@ SHORT_COMMIT="${COMMIT:0:8}" | |||
| BUCKET="${BUCKET:-k8s-staging-kind}" | |||
| # under each of these | |||
| VERSIONS=( | |||
| latest | |||
| "${SHORT_COMMIT}" | |||
| "latest" | |||
There was a problem hiding this comment.
these are arguments in an array, and we want the literal latest as a distinct word, quoting here is superfluous, as a literal it doesn't need double quotes
These changes aim to make the scripts more robust, easier to maintain, and more efficient while ensuring they follow the project's established coding patterns. (Please refer to the below key changes.)
e2e-k8s.sh
Enhanced error handling by adding set -o pipefail.
Improved code structure by extracting repeated tasks into functions.
Added informative log messages for better execution tracking.
Standardized variable usage to ${VARIABLE} format for consistency.
Retained temporary directory creation and cleanup logic for better resource management.
push-latest-cli.sh
Updated error handling to use set -o errexit -o nounset -o pipefail.
Introduced an upload_file function to reduce code duplication.
Implemented use of gsutil -m cp for parallel uploads to improve performance.
Added log messages at key steps for clearer execution tracking.
Standardized variable declarations using double quotes.