ci-operator/step-registry/baremetalds: Fix ShellCheck quoting and useless 'cat' #9774
Conversation
|
Why is rehearse testing so much? Most of what it's testing doesn't use these steps. Anyway, if we must do this, it's fine I guess. IMHO most of the shellcheck recommendation are less than worthless as they hurt readability. The most annoying of which is quoting it all for values that will never have spaces in them. /shrug |
|
@andfasano @akiselev1 @derekhiggins @honza Could one of you PTAL? Thanks! |
|
/approve |
openshift-ci-robot
added
the
approved
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
I do want to make sure e2e-metal-ipi runs to the end, hold can be removed when it does |
|
/test pj-rehearse |
@stbenjam can you specifically list jobs that should not have been rehearsed?
Trusting user input to never have spaces is a good way to have a real bad time ;) IMO any bash without quotes is immediately suspect |
|
@stbenjam This PR modifies the file |
I missed the change to something under e2e/test, so looks like rehearse did the right thing. That said, given the title of this PR indicates only baremetalds, that should probably have been separate. |
|
Oops, will drop the accidental file. Not sure why metal was terminated. |
…less 'cat'
Address issues like [1,2]:
In .//ci-operator/step-registry/baremetalds/devscripts/conf/compact/baremetalds-devscripts-conf-compact-commands.sh line 9:
echo "export NUM_WORKERS=0" >> ${SHARED_DIR}/dev-scripts-additional-config
^-----------^ SC2086: Double quote to prevent globbing and word splitting.
Did you mean:
echo "export NUM_WORKERS=0" >> "${SHARED_DIR}"/dev-scripts-additional-config
For more information:
https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...
and [1,2]:
In .//ci-operator/step-registry/baremetalds/packet/check/baremetalds-packet-check-commands.sh line 9:
PACKET_PROJECT_ID=$(cat ${CLUSTER_PROFILE_DIR}/.packet-kni-vars|grep packet_project_id|awk '{print $2}')
^--------------------^ SC2086: Double quote to prevent globbing and word splitting.
^-- SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.
...
https://www.shellcheck.net/wiki/SC2002 -- Useless cat. Consider 'cmd < file...
and:
In .//ci-operator/step-registry/baremetalds/devscripts/gather/baremetalds-devscripts-gather-commands.sh line 26:
SSHOPTS=(-o ConnectTimeout=5 -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -o ServerAliveInterval=90 -i "${CLUSTER_PROFILE_DIR}/.packet-kni-ssh-privatekey")
^--------------^ SC2191: The = here is literal. To assign by index, use ( [index]=value ) with no spaces. To keep as literal, quote it.
^----------------------^ SC2191: The = here is literal. To assign by index, use ( [index]=value ) with no spaces. To keep as literal, quote it.
^--------------------------^ SC2191: The = here is literal. To assign by index, use ( [index]=value ) with no spaces. To keep as literal, quote it.
^--------------------^ SC2191: The = here is literal. To assign by index, use ( [index]=value ) with no spaces. To keep as literal, quote it.
[1]: https://deck-ci.apps.ci.l2s4.p1.openshiftapps.com/view/gcs/origin-ci-test/pr-logs/pull/openshift_release/9772/pull-ci-openshift-release-master-step-registry-shellcheck/1273670881133989888#1:build-log.txt%3A2
[2]: https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_release/9772/pull-ci-openshift-release-master-step-registry-shellcheck/1273670881133989888/build-log.txt
wking
force-pushed
the
metal-shellcheck-cleanup
branch
from
b23ca51 to
cd781ad
Compare
|
Dropped the |
|
Both rehearsals died on install with something like: Not clear to me yet if that's something I broke or not... |
|
@wking: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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. I understand the commands that are listed here. |
|
|
|
So are we comfortable with the changes I'm making here, or do I need to change something? |
|
Yes, these changes are fine - #9774 (comment) I only wanted to see it run to the end. /hold cancel |
openshift-ci-robot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: honza, stbenjam, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@wking: Updated the following 2 configmaps:
DetailsIn response to this:
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. |
8 participants
Address issues like:
and:
and:
in preparation for #9772.