-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
verify-generated-files-remake.sh: fix issues reported by shellcheck (part 2) #73690
verify-generated-files-remake.sh: fix issues reported by shellcheck (part 2) #73690
Conversation
Hi @ipuustin. Thanks for your PR. I'm waiting for a kubernetes 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/test-infra repository. |
/cc @ixdy @BenTheElder |
/sig testing |
64d7726
to
2e64a10
Compare
@ixdy Please take another look! |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ipuustin, ixdy 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 |
/retest Review the full test history for this PR. Silence the bot with an |
1 similar comment
/retest Review the full test history for this PR. Silence the bot with an |
/hold CI says there's still one shellcheck issue remaining:
This is a new bug not present in the file when I first made the PR, but must have been introduced when I rebased after fixing the review issues. :-) Will fix and push again. |
Move away from arrays to strings to fix several shellcheck-reported issues. It isn't useful to expand the found files into arrays, because only things that are checked are if the array is empty or the contents of the first array item. Fix also a shellcheck issue about using a literal string as regexp match. It appears that the original reason for using a regexp was to avoid specifying the directory in which the script is run. However, due to the need of calling 'make generated_files', the directory is fixed anyway, and the regexp can be left out. Testing the change can be done with the following script which emulates the different cases which the script can see. In the output the variable 'X' is the array and 'Z' is the string. #!/bin/bash set -o errexit set -o nounset set -o pipefail function find_genfiles() { find . \ \( \ -not \( \ \( \ -path ./_\* -o \ -path ./.\* \ \) -prune \ \) \ \) -name "$1" } # $1 = filename pattern as in "zz_generated.$1.go" # $2 timestamp file function newer() { find_genfiles "$1" | while read -r F; do if [[ "${F}" -nt "$2" ]]; then echo "${F}" fi done } STAMP=stamp mkdir -p xxx touch xxx/foobar touch "${STAMP}" mkdir -p foo touch foo/foobar mkdir -p bar touch bar/foobar # two newer files X=($(newer foobar "${STAMP}")) if [[ "${#X[*]}" != 0 ]]; then echo "X1:" echo " ${X[*]:-(none)}" fi Z="$(newer foobar "${STAMP}")" if [[ -n "$Z" ]]; then echo "Z1:" echo " ${Z}" | tr '\n' ' ' echo "" fi # no newer files touch "${STAMP}" X=($(newer foobar "${STAMP}")) if [[ "${#X[*]}" != 0 ]]; then echo "X2:" echo " ${X[*]:-(none)}" fi Z="$(newer foobar "${STAMP}")" if [[ -n "$Z" ]]; then echo "Z2:" echo " ${Z}" | tr '\n' ' ' echo "" fi # one newer file, name matches touch "${STAMP}" touch bar/foobar X=($(newer foobar "${STAMP}")) if [[ "${#X[@]}" != 1 || ! ( "${X[0]}" =~ "bar/foobar" ) ]]; then echo "X3:" echo " ${X[*]:-(none)}" fi Z="$(newer foobar "${STAMP}")" if [[ -z "${Z}" || ${Z} != "./bar/foobar" ]]; then echo "Z3:" echo " ${Z:-(none)}" | tr '\n' ' ' echo "" fi # one newer file, name doesn't match touch "${STAMP}" touch foo/foobar X=($(newer foobar "${STAMP}")) if [[ "${#X[@]}" != 1 || ! ( "${X[0]}" =~ "bar/foobar" ) ]]; then echo "X4:" echo " ${X[*]:-(none)}" fi Z="$(newer foobar "${STAMP}")" if [[ -z "${Z}" || ${Z} != "./bar/foobar" ]]; then echo "Z4:" echo " ${Z:-(none)}" | tr '\n' ' ' echo "" fi The expected output from running this script: X1: ./bar/foobar ./foo/foobar Z1: ./bar/foobar ./foo/foobar X4: ./foo/foobar Z4: ./foo/foobar
Fix a shellcheck error by quoting a return value properly.
2e64a10
to
f1cb820
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
thanks
/hold cancel |
/retest |
/kind cleanup
What this PR does / why we need it:
Shellcheck cleanups for
hack/verify-generated-files-remake.sh
. Change the script logic a bit to fixes issues related to array creation by removing unnecessary arrays. Remove regexps too. Continues work started in #73464. The second patch contains a script for validating the assumptions made in script logic changes.Does this PR introduce a user-facing change?: