-
Notifications
You must be signed in to change notification settings - Fork 31
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
start-ceph: do rm -rf only when path exists #88
base: main
Are you sure you want to change the base?
Conversation
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.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
shellcheck
🚫 [shellcheck] reported by reviewdog 🐶
Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @). SC2199
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 62 in fccae8d
if [[ -n "$@" ]]; then |
Assigning an array to a string! Assign as array, or use * instead of @ to concatenate. SC2124
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 108 in fccae8d
TOX_ARGS="$@" |
[shellcheck] reported by reviewdog 🐶
Consider using 'grep -c' instead of 'grep|wc -l'. SC2126
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 116 in fccae8d
if [[ "$(tox -l | grep cov | wc -l)" > 0 ]]; then # Nautilus branch. |
🚫 [shellcheck] reported by reviewdog 🐶
> is for string comparisons. Use -gt instead. SC2071
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 116 in fccae8d
if [[ "$(tox -l | grep cov | wc -l)" > 0 ]]; then # Nautilus branch. |
📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 125 in fccae8d
tox ${TOX_OPTIONS} $TOX_ARGS |
Declare and assign separately to avoid masking return values. SC2155
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 147 in fccae8d
local grafonnet_version=$(grep 'set(ver' CMakeLists.txt | grep -Eo "([0-9.])+") |
Assigning an array to a string! Assign as array, or use * instead of @ to concatenate. SC2124
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 181 in fccae8d
MYPY_ARGS="$@" |
📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 186 in fccae8d
mypy --config-file="$MYPY_CONFIG_FILE" --cache-dir=src/.mypy_cache --follow-imports=skip ${MYPY_ARGS} |
📝 [shellcheck] reported by reviewdog 🐶
Not following: ./run-backend-api-tests.sh: openBinaryFile: does not exist (No such file or directory) SC1091
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 228 in fccae8d
source ./run-backend-api-tests.sh |
Assigning an array to a string! Assign as array, or use * instead of @ to concatenate. SC2124
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 255 in fccae8d
E2E_CMD="npx cypress run $@ --browser chrome --headless" |
[shellcheck] reported by reviewdog 🐶
Consider using 'grep -c' instead of 'grep|wc -l'. SC2126
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 256 in fccae8d
elif [[ "$(npm run | grep e2e:ci | wc -l)" == 1 ]]; then |
📝 [shellcheck] reported by reviewdog 🐶
Consider using pgrep instead of grepping ps output. SC2009
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 261 in fccae8d
if [[ $(ps -ef | grep -v grep | grep "ceph-mgr -i" | wc -l) == 0 ]]; then |
[shellcheck] reported by reviewdog 🐶
Consider using 'grep -c' instead of 'grep|wc -l'. SC2126
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 261 in fccae8d
if [[ $(ps -ef | grep -v grep | grep "ceph-mgr -i" | wc -l) == 0 ]]; then |
Declare and assign separately to avoid masking return values. SC2155
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 270 in fccae8d
export DASHBOARD_URL=$("$CEPH_BIN"/ceph mgr services | jq -r .dashboard) |
📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 286 in fccae8d
${E2E_CMD} -- ${ARGS} |
📝 [shellcheck] reported by reviewdog 🐶
Command appears to be unreachable. Check usage (or ignore if invoked indirectly). SC2317
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 314 in fccae8d
return 2> /dev/null || true |
📝 [shellcheck] reported by reviewdog 🐶
Command appears to be unreachable. Check usage (or ignore if invoked indirectly). SC2317
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 317 in fccae8d
"${@}" |
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.
📝 [shellcheck] reported by reviewdog 🐶
Not following: /docker/set-start-env.sh: openBinaryFile: does not exist (No such file or directory) SC1091
ceph-dev/docker/ceph/start-ceph.sh
Line 5 in fccae8d
source /docker/set-start-env.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.
The surrounding quotes actually unquote this. Remove or escape them. SC2027
ceph-dev/docker/ceph/start-ceph.sh
Line 14 in fccae8d
jq "(.[] | .target)=\""${TARGET_URL}"\"" proxy.conf.json.sample > proxy.conf.json |
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.
📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086
ceph-dev/docker/ceph/start-ceph.sh
Line 14 in fccae8d
jq "(.[] | .target)=\""${TARGET_URL}"\"" proxy.conf.json.sample > proxy.conf.json |
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.
Assigning an array to a string! Assign as array, or use * instead of @ to concatenate. SC2124
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 30 in fccae8d
local TARGET="$@" |
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.
Declare and assign separately to avoid masking return values. SC2155
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 32 in fccae8d
local CONSOLE_CALLS=$((echo "${TARGET}" | xargs grep -Eirn "console\..*\(") || echo '') |
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.
🚫 [shellcheck] reported by reviewdog 🐶
Shells disambiguate
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 32 in fccae8d
local CONSOLE_CALLS=$((echo "${TARGET}" | xargs grep -Eirn "console\..*\(") || echo '') |
an example inside a container..
|
start-ceph has a line to remove `rm -rf "$CEPH_CONF_PATH"/*` If someone run this script locally when he has root previllages, he could accidentally remove his `/` directory because `CEPH_CONF_PATH` will come empty and the command will become `rm -rf /*` which is like shooting yourself infinitely.. Signed-off-by: Nizamudeen A <[email protected]>
fccae8d
to
ebd2c1c
Compare
bot educated me about https://github.com/koalaman/shellcheck/wiki/SC2115, so going with it. |
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.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
shellcheck
Assigning an array to a string! Assign as array, or use * instead of @ to concatenate. SC2124
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 181 in ebd2c1c
MYPY_ARGS="$@" |
📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 186 in ebd2c1c
mypy --config-file="$MYPY_CONFIG_FILE" --cache-dir=src/.mypy_cache --follow-imports=skip ${MYPY_ARGS} |
📝 [shellcheck] reported by reviewdog 🐶
Not following: ./run-backend-api-tests.sh: openBinaryFile: does not exist (No such file or directory) SC1091
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 225 in ebd2c1c
source ./run-backend-api-tests.sh |
Assigning an array to a string! Assign as array, or use * instead of @ to concatenate. SC2124
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 252 in ebd2c1c
E2E_CMD="npx cypress run $@ --browser chrome --headless" |
[shellcheck] reported by reviewdog 🐶
Consider using 'grep -c' instead of 'grep|wc -l'. SC2126
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 253 in ebd2c1c
elif [[ "$(npm run | grep e2e:ci | wc -l)" == 1 ]]; then |
📝 [shellcheck] reported by reviewdog 🐶
Consider using pgrep instead of grepping ps output. SC2009
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 258 in ebd2c1c
if [[ $(ps -ef | grep -v grep | grep "ceph-mgr -i" | wc -l) == 0 ]]; then |
[shellcheck] reported by reviewdog 🐶
Consider using 'grep -c' instead of 'grep|wc -l'. SC2126
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 258 in ebd2c1c
if [[ $(ps -ef | grep -v grep | grep "ceph-mgr -i" | wc -l) == 0 ]]; then |
Declare and assign separately to avoid masking return values. SC2155
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 267 in ebd2c1c
export DASHBOARD_URL=$("$CEPH_BIN"/ceph mgr services | jq -r .dashboard) |
📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 283 in ebd2c1c
${E2E_CMD} -- ${ARGS} |
📝 [shellcheck] reported by reviewdog 🐶
Command appears to be unreachable. Check usage (or ignore if invoked indirectly). SC2317
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 311 in ebd2c1c
return 2> /dev/null || true |
📝 [shellcheck] reported by reviewdog 🐶
Command appears to be unreachable. Check usage (or ignore if invoked indirectly). SC2317
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 314 in ebd2c1c
"${@}" |
@@ -45,7 +45,7 @@ if [[ -n "${DASHBOARD_URL}" ]]; then | |||
exit 0 | |||
fi | |||
|
|||
rm -rf "$CEPH_CONF_PATH"/* | |||
rm -rf "${CEPH_CONF_PATH:?}"/* | |||
|
|||
cd /ceph/build | |||
../src/vstart.sh ${VSTART_OPTIONS} |
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 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.
Declare and assign separately to avoid masking return values. SC2155
ceph-dev/docker/ceph/start-ceph.sh
Line 75 in ebd2c1c
readonly VSTART_HAS_SSL_FLAG=$(cat /ceph/src/vstart.sh | grep DASHBOARD_SSL | wc -l) |
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.
[shellcheck] reported by reviewdog 🐶
Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead. SC2002
ceph-dev/docker/ceph/start-ceph.sh
Line 75 in ebd2c1c
readonly VSTART_HAS_SSL_FLAG=$(cat /ceph/src/vstart.sh | grep DASHBOARD_SSL | wc -l) |
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.
[shellcheck] reported by reviewdog 🐶
Consider using 'grep -c' instead of 'grep|wc -l'. SC2126
ceph-dev/docker/ceph/start-ceph.sh
Line 75 in ebd2c1c
readonly VSTART_HAS_SSL_FLAG=$(cat /ceph/src/vstart.sh | grep DASHBOARD_SSL | wc -l) |
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.
📝 [shellcheck] reported by reviewdog 🐶
Possible misspelling: DASHBOARD_SSL may not be assigned. Did you mean DASHBOARD_URL? SC2153
ceph-dev/docker/ceph/start-ceph.sh
Line 76 in ebd2c1c
if [[ "$DASHBOARD_SSL" == 0 && "$VSTART_HAS_SSL_FLAG" == 0 ]]; then |
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.
🚫 [shellcheck] reported by reviewdog 🐶
Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @). SC2199
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 62 in ebd2c1c
if [[ -n "$@" ]]; then |
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.
Assigning an array to a string! Assign as array, or use * instead of @ to concatenate. SC2124
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 108 in ebd2c1c
TOX_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.
[shellcheck] reported by reviewdog 🐶
Consider using 'grep -c' instead of 'grep|wc -l'. SC2126
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 116 in ebd2c1c
if [[ "$(tox -l | grep cov | wc -l)" > 0 ]]; then # Nautilus branch. |
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.
🚫 [shellcheck] reported by reviewdog 🐶
> is for string comparisons. Use -gt instead. SC2071
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 116 in ebd2c1c
if [[ "$(tox -l | grep cov | wc -l)" > 0 ]]; then # Nautilus branch. |
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.
📝 [shellcheck] reported by reviewdog 🐶
Double quote to prevent globbing and word splitting. SC2086
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 125 in ebd2c1c
tox ${TOX_OPTIONS} $TOX_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.
Declare and assign separately to avoid masking return values. SC2155
ceph-dev/docker/ceph/ci/sanity-checks.sh
Line 147 in ebd2c1c
local grafonnet_version=$(grep 'set(ver' CMakeLists.txt | grep -Eo "([0-9.])+") |
start-ceph.sh
has a line to removerm -rf "$CEPH_CONF_PATH"/*
If someone run this script locally when he has root previllages, he could accidentally remove his/
directory becauseCEPH_CONF_PATH
will come empty and the command will becomerm -rf /*
which is like shooting yourself infinitely..