shellcheck/shfmt more files; run check-config in CI by kolyshkin · Pull Request #3378 · opencontainers/runc

kolyshkin · 2022-02-16T21:58:29Z

This almost fixes the TODO item for shellcheck.

Please review commit-by-commit; see individual commits for details.

kolyshkin · 2022-03-01T02:53:05Z

@tianon PTAL 🙏🏻

tianon

Hopefully my suggestions on these PRs aren't making you regret asking me for my thoughts on them 😂 😇

Generally speaking, I try to avoid bash-isms unless they provide obvious value (or I know for certain the script will never want or need to become /bin/sh). This makes sure that when I do have to write /bin/sh (such as in Dockerfile RUN lines 😅), I'm not constantly getting the syntax wrong because my muscle memory is already mostly right. 😄

tianon · 2022-03-02T01:15:46Z

 search_config() {
-	local target_dir="$1"
-	[[ "$target_dir" ]] || target_dir=("${possibleConfigs[@]}")
+	local target_dir=("${1:-${possibleConfigs[@]}}")


I wasn't sure if the quoting here would have the right effect, but it appears that it does!

$ arr=( 'foo bar' 'baz' ); foo() { local foo=("${1:-${arr[@]}}"); local bar; for bar in "${foo[@]}"; do echo "$bar"; done; }; foo bar; foo bar foo bar baz

tianon · 2022-03-02T01:19:19Z

 }

-if [ "$kernelMajor" -lt 4 ] || [ "$kernelMajor" -eq 4 -a "$kernelMinor" -le 5 ]; then
+if [ "$kernelMajor" -lt 4 ] || [[ "$kernelMajor" -eq 4 && "$kernelMinor" -le 5 ]]; then


Suggested change

if [ "$kernelMajor" -lt 4 ] || [[ "$kernelMajor" -eq 4 && "$kernelMinor" -le 5 ]]; then

if [ "$kernelMajor" -lt 4 ] || { [ "$kernelMajor" -eq 4 ] && [ "$kernelMinor" -le 5 ]; }; then

kolyshkin · 2022-03-09T02:32:07Z

@tianon addressed all your comments; PTAL

tianon · 2022-03-09T22:49:26Z

Nice! I love how much cleaner your kernel_lt function makes those checks 😍

kolyshkin · 2022-03-14T16:37:27Z

@thaJeztah @AkihiroSuda PTAL

kolyshkin · 2022-03-21T18:30:20Z

@thaJeztah @AkihiroSuda PTAL (this was already blessed by @tianon)

thaJeztah

LGTM, nice!

kolyshkin · 2022-03-23T03:01:56Z

Rebased (no changed, just a merge conflict in .cirrus.yml). PTAL @thaJeztah @AkihiroSuda 🙏🏻

thaJeztah

still LGTM

…and fix a single format issue found. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

1. Allow wrap_bad and wrap_good to have an optional arguments. 2. Remove unneeded echos; this fixes the shellcheck warnings like In ./script/check-config.sh line 178: echo "$(wrap_bad 'cgroup hierarchy' 'nonexistent??')" ^-- SC2005 (style): Useless echo? Instead of 'echo $(cmd)', just use 'cmd'. 3. Fix missing color argument in calls to wrap_color (when printing the hint about how to install apparmor). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Like this one: In ./script/check-config.sh line 215: if [ "$kernelMajor" -lt 5 ] || [ "$kernelMajor" -eq 5 -a "$kernelMinor" -le 1 ]; then ^-- SC2166 (warning): Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

... and add this file to shellcheck target in Makefile. These: In script/check-config.sh line 27: kernelMinor="${kernelVersion#$kernelMajor.}" ^----------^ SC2295 (info): Expansions inside ${..} need to be quoted separately, otherwise they match as patterns. Did you mean: kernelMinor="${kernelVersion#"$kernelMajor".}" In script/check-config.sh line 103: source /etc/os-release 2>/dev/null || /bin/true ^-------------^ SC1091 (info): Not following: /etc/os-release was not specified as input (see shellcheck -x). In script/check-config.sh line 267: NET_CLS_CGROUP $netprio ^------^ SC2206 (warning): Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

One particularly bad one is ${codes[@]} which is fine in bash 4.4+, but gives "codes[@]: unbound variable" with older bash versions, such as with bash 4.2 used on CentOS 6. It's good that this is the only array in the script that can potentially be empty. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This is done to make sure the script is working correctly in different environments (distro and kernel versions). In addition, we can see in test logs which kernel features are enabled. Note that I didn't want to have a separate job for GHA CI, so I just added this to the end of shellcheck one. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Now the only remaining file that needs shellcheck warnings to be fixed is bash-completion. Note that in Makefile's TODO. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2022-03-31T03:48:05Z

Rebased; PTAL @AkihiroSuda

kolyshkin · 2022-04-14T01:01:03Z

Do I still need to address anything in this PR? PTAL @thaJeztah @AkihiroSuda @opencontainers/runc-maintainers

thaJeztah

LGTM

kolyshkin added the area/ci label Feb 16, 2022

kolyshkin force-pushed the ci-verify-more branch 2 times, most recently from 7573e7e to 60581d8 Compare February 16, 2022 22:32

kolyshkin changed the title ~~shellcheck/shfmt more files; run check-config in CI~~ shellcheck/shfmt more files; run check-config in CI; bump shfmt Feb 16, 2022

kolyshkin force-pushed the ci-verify-more branch from 60581d8 to ff7541c Compare February 16, 2022 22:33

kolyshkin changed the title ~~shellcheck/shfmt more files; run check-config in CI; bump shfmt~~ shellcheck/shfmt more files; run check-config in CI Feb 16, 2022

kolyshkin force-pushed the ci-verify-more branch from ff7541c to eabfd7f Compare February 17, 2022 02:04

tianon reviewed Mar 2, 2022

View reviewed changes

kolyshkin force-pushed the ci-verify-more branch from eabfd7f to f74a613 Compare March 9, 2022 02:31

kolyshkin force-pushed the ci-verify-more branch from f74a613 to 486c737 Compare March 9, 2022 02:46

kolyshkin requested review from AkihiroSuda and thaJeztah March 10, 2022 03:00

thaJeztah previously approved these changes Mar 21, 2022

View reviewed changes

kolyshkin dismissed thaJeztah’s stale review via 478989e March 23, 2022 03:01

kolyshkin force-pushed the ci-verify-more branch from 486c737 to 478989e Compare March 23, 2022 03:01

kolyshkin requested a review from thaJeztah March 23, 2022 03:02

thaJeztah previously approved these changes Mar 23, 2022

View reviewed changes

kolyshkin added 7 commits March 30, 2022 20:47

shfmt: add more files

6b16d00

…and fix a single format issue found. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

man/*sh: fix shellcheck warnings, add to shellcheck

ae6cb65

Now the only remaining file that needs shellcheck warnings to be fixed is bash-completion. Note that in Makefile's TODO. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the ci-verify-more branch from 478989e to ae6cb65 Compare March 31, 2022 03:47

kolyshkin dismissed thaJeztah’s stale review via ae6cb65 March 31, 2022 16:36

kolyshkin requested a review from thaJeztah April 4, 2022 18:07

kolyshkin added this to the 1.2.0 milestone Apr 4, 2022

AkihiroSuda approved these changes Apr 14, 2022

View reviewed changes

thaJeztah approved these changes Apr 14, 2022

View reviewed changes

thaJeztah merged commit 727aa42 into opencontainers:main Apr 14, 2022

kolyshkin deleted the ci-verify-more branch May 11, 2022 21:31

kolyshkin mentioned this pull request Apr 5, 2023

[1.1] refresh ci #3791

Merged

	if [ "$kernelMajor" -lt 4 ] \|\| [[ "$kernelMajor" -eq 4 && "$kernelMinor" -le 5 ]]; then
	if [ "$kernelMajor" -lt 4 ] \|\| { [ "$kernelMajor" -eq 4 ] && [ "$kernelMinor" -le 5 ]; }; then

Conversation

kolyshkin commented Feb 16, 2022

Uh oh!

kolyshkin commented Mar 1, 2022

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tianon Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tianon Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kolyshkin commented Mar 9, 2022

Uh oh!

tianon commented Mar 9, 2022

Uh oh!

kolyshkin commented Mar 14, 2022

Uh oh!

kolyshkin commented Mar 21, 2022

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Mar 31, 2022

Uh oh!

kolyshkin commented Apr 14, 2022

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kolyshkin commented Mar 23, 2022 •

edited

Loading