-
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
Fix shellcheck failures of hack/verify-boilerplate.sh verify-cli-conventions.sh verify-codegen.sh #76835
Fix shellcheck failures of hack/verify-boilerplate.sh verify-cli-conventions.sh verify-codegen.sh #76835
Conversation
/assign @fejta @BenTheElder |
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
/approve
hack/verify-boilerplate.sh
Outdated
files_need_boilerplate=() | ||
while IFS=$'\n' read -r line; do | ||
files_need_boilerplate+=( "$line" ) | ||
done < <(${boiler} "$@") |
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.
Maybe $boiler should be in quotes?
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.
@fejta
I checked the details of the failure, which was caused by the conflict of hack/.shellcheck_failures
.
I've updated the code now.
${boiler}
doesn't have to be in double quotes, the shellcheck test can pass.
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.
yes but it should be quoted, this will not work correctly if there's a space in $BASH_SOURCE (try mkdir -p "/tmp/very exciting dir/" && mv kubernetes "/tmp/very exciting dir" && cd "/tmp/very exciting dir" && hack/verify-boilerplate.sh
)
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.
Thanks a lot @fejta !
Updated.
[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 |
b3fb9f0
to
b297e84
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
hack/verify-boilerplate.sh
Outdated
files_need_boilerplate=() | ||
while IFS=$'\n' read -r line; do | ||
files_need_boilerplate+=( "$line" ) | ||
done < <(${boiler} "$@") |
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.
yes but it should be quoted, this will not work correctly if there's a space in $BASH_SOURCE (try mkdir -p "/tmp/very exciting dir/" && mv kubernetes "/tmp/very exciting dir" && cd "/tmp/very exciting dir" && hack/verify-boilerplate.sh
)
…entions.sh verify-codegen.sh
b297e84
to
2704abf
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
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Fix shellcheck failures of hack/verify-boilerplate.sh verify-cli-conventions.sh verify-codegen.sh
Which issue(s) this PR fixes:
Ref #72956
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
/sig testing