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.

Sign up for GitHub

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

Jump to bottom

Hint shellcheck that bash is used for all test scripts #13776

Merged
merged 5 commits into from
Jul 17, 2024
Merged

Hint shellcheck that bash is used for all test scripts #13776

merged 5 commits into from
Jul 17, 2024

Conversation

simondeziel
Copy link
Member

@simondeziel simondeziel commented Jul 16, 2024 •
edited
Loading

Also, since we are using bash, stop silencing the local is not supported by sh as it works now.

@simondeziel simondeziel mentioned this pull request Jul 16, 2024
Merged
@tomponline
Copy link
Member

Can be rebased now

@simondeziel
test: use bash and separately set options
64132cb 
bash brings interesting features without being an unreasonable minimum requirement.
Besides, `make static-analysis` already uses `shellcheck --shell bash`.

Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
@simondeziel simondeziel force-pushed the bash-tests branch 2 times, most recently from 362859e to 90ebbbc Compare July 16, 2024 20:33
@simondeziel
Add a shellcheck RC to default to bash flavor
76aaf1c 
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 16, 2024
View reviewed changes
test/suites/clustering.sh Outdated
@@ -2132,7 +2111,7 @@
ip a add 127.0.1.1"${ipRand}"/32 dev "${prefix}1"

# Create forkdns config directory
mkdir "${lxdDir}"/networks/lxdtest1/forkdns.servers -p
mkdir "${LXD_DIR}"/networks/lxdtest1/forkdns.servers -p

Check warning

Code scanning / shellcheck

SC2031 Warning test

LXD_DIR was modified in a subshell. That change might be lost.
test/suites/clustering.sh Outdated
@@ -2144,7 +2123,7 @@
ip a add 127.0.1.2"${ipRand}"/32 dev "${prefix}2"

# Create forkdns config directory
mkdir "${lxdDir}"/networks/lxdtest2/forkdns.servers -p
mkdir "${LXD_DIR}"/networks/lxdtest2/forkdns.servers -p

Check warning

Code scanning / shellcheck

SC2031 Warning test

LXD_DIR was modified in a subshell. That change might be lost.
test/suites/clustering.sh Outdated
@@ -2154,11 +2133,11 @@
sleep 1

# Create servers list file for forkdns1 pointing at forkdns2 (should be live reloaded)
echo "127.0.1.2${ipRand}" > "${lxdDir}"/networks/lxdtest1/forkdns.servers/servers.conf.tmp
mv "${lxdDir}"/networks/lxdtest1/forkdns.servers/servers.conf.tmp "${lxdDir}"/networks/lxdtest1/forkdns.servers/servers.conf
echo "127.0.1.2${ipRand}" > "${LXD_DIR}"/networks/lxdtest1/forkdns.servers/servers.conf.tmp

Check warning

Code scanning / shellcheck

SC2031 Warning test

LXD_DIR was modified in a subshell. That change might be lost.
test/suites/clustering.sh Outdated
echo "127.0.1.2${ipRand}" > "${lxdDir}"/networks/lxdtest1/forkdns.servers/servers.conf.tmp
mv "${lxdDir}"/networks/lxdtest1/forkdns.servers/servers.conf.tmp "${lxdDir}"/networks/lxdtest1/forkdns.servers/servers.conf
echo "127.0.1.2${ipRand}" > "${LXD_DIR}"/networks/lxdtest1/forkdns.servers/servers.conf.tmp
mv "${LXD_DIR}"/networks/lxdtest1/forkdns.servers/servers.conf.tmp "${LXD_DIR}"/networks/lxdtest1/forkdns.servers/servers.conf

Check warning

Code scanning / shellcheck

SC2031 Warning test

LXD_DIR was modified in a subshell. That change might be lost.
test/suites/clustering.sh Outdated
echo "127.0.1.2${ipRand}" > "${lxdDir}"/networks/lxdtest1/forkdns.servers/servers.conf.tmp
mv "${lxdDir}"/networks/lxdtest1/forkdns.servers/servers.conf.tmp "${lxdDir}"/networks/lxdtest1/forkdns.servers/servers.conf
echo "127.0.1.2${ipRand}" > "${LXD_DIR}"/networks/lxdtest1/forkdns.servers/servers.conf.tmp
mv "${LXD_DIR}"/networks/lxdtest1/forkdns.servers/servers.conf.tmp "${LXD_DIR}"/networks/lxdtest1/forkdns.servers/servers.conf

Check warning

Code scanning / shellcheck

SC2031 Warning test

LXD_DIR was modified in a subshell. That change might be lost.
test/suites/clustering.sh Outdated

# Create fake DHCP lease file on forkdns2 network
echo "$(date +%s) 00:16:3e:98:05:40 10.140.78.145 test1 ff:2b:a8:0a:df:00:02:00:00:ab:11:36:ea:11:e5:37:e0:85:45" > "${lxdDir}"/networks/lxdtest2/dnsmasq.leases
echo "$(date +%s) 00:16:3e:98:05:40 10.140.78.145 test1 ff:2b:a8:0a:df:00:02:00:00:ab:11:36:ea:11:e5:37:e0:85:45" > "${LXD_DIR}"/networks/lxdtest2/dnsmasq.leases

Check warning

Code scanning / shellcheck

SC2031 Warning test

LXD_DIR was modified in a subshell. That change might be lost.
@simondeziel
test: stop silencing local is not supported by sh
a976147 
Those are shellcheck: SC2039 and SC3043

This is supported now that we use `bash`. Change done mechanically with:

```
find test/ -type f -name '*.sh' -exec sed -i '/# shellcheck disable=\(SC\)\?2039,\(SC\)\?3043$/d' {} +
```

With manual fix-up.

Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
@simondeziel
test/suites/security: avoid reusing standard UID shell variable
0f5878a 
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
@simondeziel simondeziel force-pushed the bash-tests branch 3 times, most recently from b7b1fce to 91fc74a Compare July 17, 2024 01:44
@simondeziel
test/suites/clustering: fix variable shadowing/reuse/uninit
18fb38a 
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
@simondeziel simondeziel marked this pull request as ready for review July 17, 2024 02:20
tomponline
tomponline approved these changes Jul 17, 2024
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Thanks!

@tomponline tomponline merged commit 9982a09 into canonical:main Jul 17, 2024
28 of 29 checks passed
@simondeziel simondeziel deleted the bash-tests branch July 17, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline tomponline approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@simondeziel @tomponline