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

Run shellcheck in CI (mk 3) #97

Merged
merged 9 commits into from Jul 13, 2023
Merged

Run shellcheck in CI (mk 3) #97

merged 9 commits into from Jul 13, 2023

Conversation

yamanawabi
Copy link
Contributor

For issue #72

Shellcheck workflow downloads latest version from spellcheck repo. Currently, ubuntu provides 0.8.0. Latest is 0.9.0.

For try fixes:

In try line 68:
            mkdir -p "$SANDBOX_DIR"/upperdir/"$mountpoint" "$SANDBOX_DIR"/workdir"/$mountpoint" "$SANDBOX_DIR"/temproot/"$mountpoint"
                                                                         ^------^ SC2140 (warning): Word is of the form "A"B"C" (B indicated). Did you mean "ABC" or "A\"B\"C"?

Fixed issue by removing extra quotes and using curly braces with variables

In try line 73:
    export chroot_executable=$(mktemp)
           ^---------------^ SC2155 (warning): Declare and assign separately to avoid masking return values.
                             ^-------^ SC2046 (warning): Quote this to prevent word splitting.


In try line 74:
    export try_mount_log=$(mktemp)
           ^-----------^ SC2155 (warning): Declare and assign separately to avoid masking return values.
                         ^-------^ SC2046 (warning): Quote this to prevent word splitting.


In try line 75:
    export script_to_execute=$(mktemp)
           ^---------------^ SC2155 (warning): Declare and assign separately to avoid masking return values.
                             ^-------^ SC2046 (warning): Quote this to prevent word splitting.

Fixed by defining the variables and then exporting them

In try line 229:
            read -p "Commit these changes? [y/N] " DO_COMMIT >&2
                 ^-- SC3045 (warning): In POSIX sh, read -p is undefined.

In try line 468:
            union_helper="$OPTARG" 
            ^----------^ SC2034 (warning): union_helper appears unused. Verify use (or export if used externally).

Added shellcheck ignores for these in the workflow. $union_helper is used in the generated script, and we're dealing with Linux here specifically, so POSIX compliance is not paramount

In try line 470:
        (h|*) usage; exit 0;;
         ^-- SC2214 (warning): This case is not specified by getopts.

Added h to getops line

Copy link
Member

@angelhof angelhof left a comment

Choose a reason for hiding this comment

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

Great, thanks for your work! It looks good other than the minor nits that I added. No need to make a new PR, you can add new commits to this fixing the changes that I propose (than I will squash the PR into a single commit to main anyway). Once you do the changes @mgree approves it too we can merge :)

.github/workflows/shellcheck.yaml Outdated Show resolved Hide resolved
.github/workflows/shellcheck.yaml Outdated Show resolved Hide resolved
try Outdated Show resolved Hide resolved
@angelhof angelhof requested a review from mgree July 5, 2023 15:14
Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

Just a few nits---looks good, thanks!

.github/workflows/shellcheck.yaml Outdated Show resolved Hide resolved
@mgree mgree changed the title shellcheck workflow - third time is a charm Run shellcheck in CI (mk 3) Jul 6, 2023
@mgree mgree mentioned this pull request Jul 6, 2023
9 tasks
@yamanawabi
Copy link
Contributor Author

yamanawabi commented Jul 12, 2023

Update: With getopts set to yvnhi, the shellcheck and test workflows are now passing. The explanation on colon use indicating an expected argument helped. Brainfart has been conquered.


Alright, so with getopts "yvni:D:U:", shellcheck passes a warning about the h being unspecified by getopts: https://github.com/yamanawabi/try/actions/runs/5528024252/jobs/10084360738

With getopts "yvnih:D:U", the following test fails:
https://github.com/yamanawabi/try/actions/runs/5527993006/jobs/10084295984

/tmp/tmp.pHKuXk34Ra: 1: /tmp/tmp.MLDTXG2vfr: foo1: not found
find: ‘standard output’: Broken pipe
find: write error
diff: bar.txt: No such file or directory
Running test_ignore_flag... non-zero ec (2)		FAIL

all other requests have been fixed up

Copy link
Member

@angelhof angelhof left a comment

Choose a reason for hiding this comment

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

Tiny nit! Looks good otherwise, thanks for the help!

try Outdated Show resolved Hide resolved
@angelhof angelhof requested a review from mgree July 12, 2023 15:05
@mgree mgree merged commit 0c09d8a into binpash:main Jul 13, 2023
11 checks passed
@mgree mgree mentioned this pull request Jul 13, 2023
@yamanawabi
Copy link
Contributor Author

thanks for making the fix and merging!

@yamanawabi yamanawabi deleted the shellcheck-with-fixes branch July 25, 2023 18:05
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

3 participants