Skip to content
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 warnings #4042

Merged
merged 1 commit into from Feb 1, 2022
Merged

Fix shellcheck warnings #4042

merged 1 commit into from Feb 1, 2022

Conversation

mfundul
Copy link
Contributor

@mfundul mfundul commented Feb 1, 2022

No description provided.

@mfundul mfundul marked this pull request as ready for review February 1, 2022 11:13
@mfundul mfundul requested a review from a team as a code owner February 1, 2022 11:13
@@ -158,7 +158,8 @@ wait_for_pg() {
exit 1
}

VERSION=`echo ${UPDATE_FROM_TAG} | sed 's/\([0-9]\{0,\}\.[0-9]\{0,\}\.[0-9]\{0,\}\).*/\1/g'`
# shellcheck disable=SC2001
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# shellcheck disable=SC2001
# shellcheck disable=SC2001 -- See if you can use ${variable//search/replace} instead.

I find it convenient to add the description of what is disabled. Not sure it works in this syntax though, maybe a new line is required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Judging from their docs, should work with a second # instead of dashes

Suggested change
# shellcheck disable=SC2001
# shellcheck disable=SC2001 # See if you can use ${variable//search/replace} instead.

Comment on lines 141 to 142
# shellcheck disable=SC2034
for i in {1..20}; do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# shellcheck disable=SC2034
for i in {1..20}; do
for _ in {1..20}; do

We can use underscore for dummy variables, shellcheck won't complain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Member

@akuzm akuzm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how these warnings managed to escape the initial check...

@mfundul mfundul force-pushed the fix-shellcheck branch 2 times, most recently from 4129165 to 1eaf2c9 Compare February 1, 2022 11:25
@codecov
Copy link

codecov bot commented Feb 1, 2022

Codecov Report

Merging #4042 (ac67368) into master (ae02934) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4042      +/-   ##
==========================================
- Coverage   90.70%   90.66%   -0.04%     
==========================================
  Files         214      214              
  Lines       38726    38733       +7     
==========================================
- Hits        35126    35118       -8     
- Misses       3600     3615      +15     
Impacted Files Coverage Δ
src/chunk.h 100.00% <ø> (ø)
src/chunk.c 95.55% <100.00%> (+0.03%) ⬆️
src/planner.c 94.95% <100.00%> (+0.01%) ⬆️
src/ts_catalog/chunk_data_node.c 98.98% <100.00%> (ø)
tsl/src/fdw/modify_plan.c 90.16% <100.00%> (+0.16%) ⬆️
tsl/src/fdw/relinfo.c 96.22% <100.00%> (-0.10%) ⬇️
src/bgw/scheduler.c 83.91% <0.00%> (-2.64%) ⬇️
src/loader/bgw_launcher.c 89.50% <0.00%> (-2.47%) ⬇️
src/bgw/job.c 92.93% <0.00%> (-0.29%) ⬇️
tsl/src/nodes/data_node_dispatch.c 97.10% <0.00%> (-0.25%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 481bf8b...ac67368. Read the comment docs.

@@ -194,7 +194,7 @@ docker_pgcmd ${CONTAINER_ORIG} "CHECKPOINT;"
srcdir=$(docker exec ${CONTAINER_ORIG} /bin/bash -c 'pg_config --pkglibdir')
FILES=$(docker exec ${CONTAINER_ORIG} /bin/bash -c "ls $srcdir/timescaledb*.so")
for file in $FILES; do
docker cp ${CONTAINER_ORIG}:$file ${TEST_TMPDIR}/`basename $file`
docker cp ${CONTAINER_ORIG}:$file "${TEST_TMPDIR}/$(basename $file)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need quotes around the first parameter too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add those as well.

@@ -205,8 +205,8 @@ docker_run_vol ${CONTAINER_UPDATED} ${UPDATE_VOLUME}:/var/lib/postgresql/data ${

dstdir=$(docker exec ${CONTAINER_UPDATED} /bin/bash -c 'pg_config --pkglibdir')
for file in $FILES; do
docker cp ${TEST_TMPDIR}/`basename $file` ${CONTAINER_UPDATED}:$dstdir
rm ${TEST_TMPDIR}/`basename $file`
docker cp "${TEST_TMPDIR}/$(basename $file)" ${CONTAINER_UPDATED}:$dstdir
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need quotes around the last parameter too?

@mfundul mfundul merged commit 9248de2 into timescale:master Feb 1, 2022
@mfundul mfundul deleted the fix-shellcheck branch February 1, 2022 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants