-
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 in test-smoke.sh and test-network.sh #75573
fix shellcheck in test-smoke.sh and test-network.sh #75573
Conversation
cluster/test-network.sh
Outdated
KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/.. | ||
# export KUBE_CONFIG_FILE | ||
# use kubeconfig default file | ||
export KUBE_CONFIG_FILE="config-default.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.
Why did I do that:
In ./cluster/test-network.sh line 28:
KUBE_CONFIG_FILE="config-default.sh"
^--------------^ SC2034: KUBE_CONFIG_FILE appears unused. Verify use (or export if used externally).
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.
I'm really wary of this one, what's using this downstream, ginkgo-e2e or something?
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.
If we do not select export, we can only delete.
https://github.com/koalaman/shellcheck/wiki/SC2034
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.
currently, ginkgo-e2e.sh seems to use KUBE_CONFIG_FILE="config-test.sh".
If it is exported in test-network.sh, ginkgo-e2e.sh uses config-default.sh instead of config-test.sh.
I think deleting is better for keeping current behavior...
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.
@spiffxp what do you think?
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.
Yeah, to maintain current behaviour, we should just remove KUBE_CONFIG_FILE
from cluster/test-network.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.
Agree, let's delete please
cluster/test-smoke.sh
Outdated
|
||
TEST_ARGS="$@" | ||
TEST_ARGS=("$@") |
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.
Why did I do that:
In ./cluster/test-smoke.sh line 28:
TEST_ARGS="$@"
^--^ SC2124: Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.
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.
doing this only to expand as TEST_ARGS[0]
down below seems like needless indirection, I feel like there's got to be a better way to express this?
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.
There should be no problem with this modification.
Ref: https://github.com/koalaman/shellcheck/wiki/SC2124
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.
TEST_ARGS=("$@") | |
TEST_ARGS="$*" |
Wouldn't that work just as easy?
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.
Has been updated.
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.
Sorry, I shouldn't have revised it all based on the shellcheck example.
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.
/cc cblecker
/assign @spiffxp @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.
this is not a clear LGTM to me yet
cluster/test-network.sh
Outdated
KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/.. | ||
# export KUBE_CONFIG_FILE | ||
# use kubeconfig default file | ||
export KUBE_CONFIG_FILE="config-default.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.
I'm really wary of this one, what's using this downstream, ginkgo-e2e or something?
cluster/test-smoke.sh
Outdated
|
||
TEST_ARGS="$@" | ||
TEST_ARGS=("$@") |
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.
doing this only to expand as TEST_ARGS[0]
down below seems like needless indirection, I feel like there's got to be a better way to express this?
/retest |
8b629ca
to
763ae29
Compare
cluster/test-network.sh
Outdated
KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/.. | ||
# export KUBE_CONFIG_FILE | ||
# use kubeconfig default file | ||
export KUBE_CONFIG_FILE="config-default.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.
Yeah, to maintain current behaviour, we should just remove KUBE_CONFIG_FILE
from cluster/test-network.sh
cluster/test-smoke.sh
Outdated
|
||
TEST_ARGS="$@" | ||
TEST_ARGS="$*" |
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.
Actually, had another look at this.. there's no need to pass this through another variable. We can scrap this line completely, and where TEST_ARGS is referenced below, we can just use "$@"
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, I have updated it
update pull request update pull request update pull request update pull request
763ae29
to
0905e9a
Compare
0905e9a
to
b9470e5
Compare
cluster/test-smoke.sh
Outdated
@@ -25,8 +25,8 @@ set -o pipefail | |||
|
|||
KUBE_ROOT=$(dirname "${BASH_SOURCE[0]}")/.. | |||
|
|||
TEST_ARGS="$@" | |||
TEST_ARGS=( "$@" ) |
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.
pr has been updated.
You can refer to: https://github.com/koalaman/shellcheck/wiki/SC2124
b9470e5
to
ad5bccc
Compare
/cc @spiffxp @BenTheElder |
/priority backlog |
/test pull-kubernetes-integration |
/cc @cblecker |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: spiffxp, xichengliudui 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 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
fixshellcheck190321
Which issue(s) this PR fixes:
Ref #72956
Special notes for your reviewer:
Does this PR introduce a user-facing change?: