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

contrib: Expand shellcheck checks #160

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

contrib: Expand shellcheck checks #160

wants to merge 4 commits into from

Conversation

n8henrie
Copy link
Collaborator

No description provided.

@n8henrie n8henrie mentioned this pull request Feb 21, 2023
Merged
ryantm
ryantm reviewed Feb 21, 2023
View reviewed changes
pkgs/agenix.sh Outdated
@@ -1,5 +1,8 @@
#!/usr/bin/env bash
# shellcheck disable=SC2250,SC2292
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand, because both of these says they are optional and have to be explicitly enabled. Is there some other shellcheck config we need to add to turn on all the checks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

$ git checkout main
$ shellcheck --enable=all pkgs/agenix.sh

In pkgs/agenix.sh line 7:
  echo "$PACKAGE - edit and rekey age secret files"
        ^------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
  echo "${PACKAGE} - edit and rekey age secret files"


In pkgs/agenix.sh line 9:
  echo "$PACKAGE -e FILE [-i PRIVATE_KEY]"
        ^------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
  echo "${PACKAGE} -e FILE [-i PRIVATE_KEY]"


In pkgs/agenix.sh line 10:
  echo "$PACKAGE -r [-i PRIVATE_KEY]"
        ^------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
  echo "${PACKAGE} -r [-i PRIVATE_KEY]"


In pkgs/agenix.sh line 85:
    if [ -n "${CLEARTEXT_DIR+x}" ]
       ^-------------------------^ SC2292 (style): Prefer [[ ]] over [ ] for tests in Bash/Ksh.

Did you mean: 
    if [[ -n "${CLEARTEXT_DIR+x}" ]]


In pkgs/agenix.sh line 87:
        rm -rf "$CLEARTEXT_DIR"
                ^------------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
        rm -rf "${CLEARTEXT_DIR}"


In pkgs/agenix.sh line 89:
    if [ -n "${REENCRYPTED_DIR+x}" ]
       ^---------------------------^ SC2292 (style): Prefer [[ ]] over [ ] for tests in Bash/Ksh.

Did you mean: 
    if [[ -n "${REENCRYPTED_DIR+x}" ]]


In pkgs/agenix.sh line 91:
        rm -rf "$REENCRYPTED_DIR"
                ^--------------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
        rm -rf "${REENCRYPTED_DIR}"


In pkgs/agenix.sh line 98:
    KEYS=$( (@nixInstantiate@ --eval -E "(let rules = import $RULES; in builtins.concatStringsSep \"\n\" rules.\"$FILE\".publicKeys)" | @sedBin@ 's/"//g' | @sedBin@ 's/\\n/\n/g') | @sedBin@ '/^$/d' || exit 1)
                                                             ^----^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.
                                                                                                                 ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
    KEYS=$( (@nixInstantiate@ --eval -E "(let rules = import ${RULES}; in builtins.concatStringsSep \"\n\" rules.\"${FILE}\".publicKeys)" | @sedBin@ 's/"//g' | @sedBin@ 's/\\n/\n/g') | @sedBin@ '/^$/d' || exit 1)


In pkgs/agenix.sh line 100:
    if [ -z "$KEYS" ]
       ^------------^ SC2292 (style): Prefer [[ ]] over [ ] for tests in Bash/Ksh.
             ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
    if [[ -z "${KEYS}" ]]


In pkgs/agenix.sh line 102:
        >&2 echo "There is no rule for $FILE in $RULES."
                                       ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.
                                                ^----^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
        >&2 echo "There is no rule for ${FILE} in ${RULES}."


In pkgs/agenix.sh line 107:
    CLEARTEXT_FILE="$CLEARTEXT_DIR/$(basename "$FILE")"
                    ^------------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.
                                               ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
    CLEARTEXT_FILE="${CLEARTEXT_DIR}/$(basename "${FILE}")"


In pkgs/agenix.sh line 109:
    if [ -f "$FILE" ]
       ^------------^ SC2292 (style): Prefer [[ ]] over [ ] for tests in Bash/Ksh.
             ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
    if [[ -f "${FILE}" ]]


In pkgs/agenix.sh line 113:
            if [ -f "$HOME/.ssh/id_rsa" ]; then
               ^------------------------^ SC2292 (style): Prefer [[ ]] over [ ] for tests in Bash/Ksh.
                     ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
            if [[ -f "${HOME}/.ssh/id_rsa" ]]; then


In pkgs/agenix.sh line 114:
                DECRYPT+=(--identity "$HOME/.ssh/id_rsa")
                                      ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
                DECRYPT+=(--identity "${HOME}/.ssh/id_rsa")


In pkgs/agenix.sh line 116:
            if [ -f "$HOME/.ssh/id_ed25519" ]; then
               ^----------------------------^ SC2292 (style): Prefer [[ ]] over [ ] for tests in Bash/Ksh.
                     ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
            if [[ -f "${HOME}/.ssh/id_ed25519" ]]; then


In pkgs/agenix.sh line 117:
                DECRYPT+=(--identity "$HOME/.ssh/id_ed25519")
                                      ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
                DECRYPT+=(--identity "${HOME}/.ssh/id_ed25519")


In pkgs/agenix.sh line 121:
          echo "No identity found to decrypt $FILE. Try adding an SSH key at $HOME/.ssh/id_rsa or $HOME/.ssh/id_ed25519 or using the --identity flag to specify a file."
                                             ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.
                                                                             ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.
                                                                                                  ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
          echo "No identity found to decrypt ${FILE}. Try adding an SSH key at ${HOME}/.ssh/id_rsa or ${HOME}/.ssh/id_ed25519 or using the --identity flag to specify a file."


In pkgs/agenix.sh line 124:
        DECRYPT+=(-o "$CLEARTEXT_FILE" "$FILE")
                      ^-------------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.
                                        ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
        DECRYPT+=(-o "${CLEARTEXT_FILE}" "${FILE}")


In pkgs/agenix.sh line 126:
        cp "$CLEARTEXT_FILE" "$CLEARTEXT_FILE.before"
            ^-------------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.
                              ^-------------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
        cp "${CLEARTEXT_FILE}" "${CLEARTEXT_FILE}.before"


In pkgs/agenix.sh line 129:
    [ -t 0 ] || EDITOR='cp /dev/stdin'
    ^------^ SC2292 (style): Prefer [[ ]] over [ ] for tests in Bash/Ksh.

Did you mean: 
    [[ -t 0 ]] || EDITOR='cp /dev/stdin'


In pkgs/agenix.sh line 131:
    $EDITOR "$CLEARTEXT_FILE"
    ^-----^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.
             ^-------------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
    ${EDITOR} "${CLEARTEXT_FILE}"


In pkgs/agenix.sh line 133:
    if [ ! -f "$CLEARTEXT_FILE" ]
       ^------------------------^ SC2292 (style): Prefer [[ ]] over [ ] for tests in Bash/Ksh.
               ^-------------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
    if [[ ! -f "${CLEARTEXT_FILE}" ]]


In pkgs/agenix.sh line 135:
      echo "$FILE wasn't created."
            ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
      echo "${FILE} wasn't created."


In pkgs/agenix.sh line 138:
    [ -f "$FILE" ] && [ "$EDITOR" != ":" ] && @diffBin@ "$CLEARTEXT_FILE.before" "$CLEARTEXT_FILE" 1>/dev/null && echo "$FILE wasn't changed, skipping re-encryption." && return
    ^------------^ SC2292 (style): Prefer [[ ]] over [ ] for tests in Bash/Ksh.
          ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.
                      ^------------------^ SC2292 (style): Prefer [[ ]] over [ ] for tests in Bash/Ksh.
                         ^-----^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.
                                                         ^-------------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.
                                                                                  ^-------------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.
                                                                                                                        ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
    [[ -f "${FILE}" ]] && [[ "${EDITOR}" != ":" ]] && @diffBin@ "${CLEARTEXT_FILE}.before" "${CLEARTEXT_FILE}" 1>/dev/null && echo "${FILE} wasn't changed, skipping re-encryption." && return


In pkgs/agenix.sh line 143:
        ENCRYPT+=(--recipient "$key")
                               ^--^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
        ENCRYPT+=(--recipient "${key}")


In pkgs/agenix.sh line 144:
    done <<< "$KEYS"
              ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
    done <<< "${KEYS}"


In pkgs/agenix.sh line 147:
    REENCRYPTED_FILE="$REENCRYPTED_DIR/$(basename "$FILE")"
                      ^--------------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.
                                                   ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
    REENCRYPTED_FILE="${REENCRYPTED_DIR}/$(basename "${FILE}")"


In pkgs/agenix.sh line 149:
    ENCRYPT+=(-o "$REENCRYPTED_FILE")
                  ^---------------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
    ENCRYPT+=(-o "${REENCRYPTED_FILE}")


In pkgs/agenix.sh line 151:
    @ageBin@ "${ENCRYPT[@]}" <"$CLEARTEXT_FILE" || exit 1
                               ^-------------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
    @ageBin@ "${ENCRYPT[@]}" <"${CLEARTEXT_FILE}" || exit 1


In pkgs/agenix.sh line 153:
    mv -f "$REENCRYPTED_FILE" "$1"
           ^---------------^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
    mv -f "${REENCRYPTED_FILE}" "$1"


In pkgs/agenix.sh line 157:
    FILES=$( (@nixInstantiate@ --eval -E "(let rules = import $RULES; in builtins.concatStringsSep \"\n\" (builtins.attrNames rules))"  | @sedBin@ 's/"//g' | @sedBin@ 's/\\n/\n/g') || exit 1)
                                                              ^----^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
    FILES=$( (@nixInstantiate@ --eval -E "(let rules = import ${RULES}; in builtins.concatStringsSep \"\n\" (builtins.attrNames rules))"  | @sedBin@ 's/"//g' | @sedBin@ 's/\\n/\n/g') || exit 1)


In pkgs/agenix.sh line 159:
    for FILE in $FILES
                ^----^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
    for FILE in ${FILES}


In pkgs/agenix.sh line 161:
        echo "rekeying $FILE..."
                       ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
        echo "rekeying ${FILE}..."


In pkgs/agenix.sh line 162:
        EDITOR=: edit "$FILE"
                       ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
        EDITOR=: edit "${FILE}"


In pkgs/agenix.sh line 167:
[ $REKEY -eq 1 ] && rekey && exit 0
^--------------^ SC2292 (style): Prefer [[ ]] over [ ] for tests in Bash/Ksh.
  ^----^ SC2248 (style): Prefer double quoting even when variables don't contain special characters.
  ^----^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
[[ "$REKEY" -eq 1 ]] && rekey && exit 0


In pkgs/agenix.sh line 168:
edit "$FILE" && cleanup && exit 0
      ^---^ SC2250 (style): Prefer putting braces around variable references even when not strictly required.

Did you mean: 
edit "${FILE}" && cleanup && exit 0

For more information:
  https://www.shellcheck.net/wiki/SC2248 -- Prefer double quoting even when v...
  https://www.shellcheck.net/wiki/SC2250 -- Prefer putting braces around vari...
  https://www.shellcheck.net/wiki/SC2292 -- Prefer [[ ]] over [ ] for tests i...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(I have enable=all in my ~/.shellcheckrc, not terribly well documented but mentioned in man 1 shellcheck)

Copy link
Owner

Choose a reason for hiding this comment

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

I think we need to enable all in our CI too. And I think I'd rather fix those two than turn them off. We don't have that nix substitution pain to worry about anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I'm happy to work on that.

@n8henrie
Copy link
Collaborator Author

I think in general that enabling all makes sense, but the recommendation to always use ${braces} is hard to use with nix, and it seems like you prefer the single [ tests. Disabling those two tests gives us a clean shellcheck --enable=all though.

@n8henrie n8henrie changed the title Fix some shellcheck warnings that don't work well with nix / this package's style contrib: Expand shellcheck checks Feb 22, 2023
@n8henrie
Copy link
Collaborator Author

Too many merge conflicts, just force-pushed overtop.

@ryantm would you also consider something like shfmt to help enforce consistent styling? I have to admit I am a big fan of black, gofmt, rustfmt, and shfmt has filled that role for me with bash / shell.

@n8henrie
Copy link
Collaborator Author

Because we use set -e, shellcheck complains about basically every && condition (because && is essentially assumed). I could unroll all of these or we could disable that check. Preference?

@ryantm
Copy link
Owner

ryantm commented Feb 22, 2023

Yeah I'm in favor of using formatters. https://github.com/numtide/treefmt seems like a cool project from fellow nix people.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryantm ryantm ryantm left review comments

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@n8henrie @ryantm