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

Use double quotes consistently for shell scripts #14938

Merged
merged 1 commit into from
Dec 12, 2024
Merged

Conversation

dhruvmanila
Copy link
Member

Summary

The release failed (https://github.com/astral-sh/ruff/actions/runs/12298190472/job/34321509636) because the shell script in the Docker release workflow was using single quotes instead of double quotes.

This is related to https://www.shellcheck.net/wiki/SC2016. I found it via actionlint. Related #14893.

I also went ahead and fixed https://www.shellcheck.net/wiki/SC2086 which were raised in a couple of places.

@dhruvmanila dhruvmanila added the release Related to the release process label Dec 12, 2024
@dhruvmanila dhruvmanila requested a review from dylwil3 December 12, 2024 14:37
Comment on lines +146 to +147
"$(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON")" \
"$(printf "${RUFF_BASE_IMG}@sha256:%s " *)"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is where the release failure happened.

Comment on lines +291 to +292
"$(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON")" \
"$(printf "${RUFF_BASE_IMG}@sha256:%s " *)"
Copy link
Member Author

Choose a reason for hiding this comment

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

And here as well.

@@ -268,9 +268,9 @@ jobs:
RELEASE_COMMIT: "${{ github.sha }}"
run: |
# Write and read notes from a file to avoid quoting breaking things
echo "$ANNOUNCEMENT_BODY" > $RUNNER_TEMP/notes.txt
echo "$ANNOUNCEMENT_BODY" > "$RUNNER_TEMP/notes.txt"
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this file auto generated?

Copy link
Member

Choose a reason for hiding this comment

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

it is

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, right. Not sure why one is quoted while the other isn't. I'll revert.

@@ -143,8 +143,8 @@ jobs:
# The final command becomes `docker buildx imagetools create -t tag1 -t tag2 ... <RUFF_BASE_IMG>@sha256:<sha256_1> <RUFF_BASE_IMG>@sha256:<sha256_2> ...`
run: |
docker buildx imagetools create \
$(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON") \
$(printf '${RUFF_BASE_IMG}@sha256:%s ' *)
"$(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON")" \
Copy link
Member

Choose a reason for hiding this comment

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

Are the nested double quotes here not an issue because they're inside of a $( ) construct? I would have naively assumed you'd need to backslash-escape them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think no because $( ) should start a new context but I think in this specific case I should revert because of the glob.

@dylwil3 dylwil3 merged commit 3629cbf into main Dec 12, 2024
26 checks passed
@dylwil3 dylwil3 deleted the dhruv/double-quote branch December 12, 2024 14:45
@AlexWaygood
Copy link
Member

AlexWaygood commented Dec 12, 2024

Is it worth adding shellcheck to our pre-commit config so we can get these issues flagged in CI in the future? https://github.com/koalaman/shellcheck?tab=readme-ov-file#pre-commit

@AlexWaygood
Copy link
Member

Thanks for fixing -- sorry for the breakage :(

@dhruvmanila
Copy link
Member Author

Is it worth adding shellcheck to our pre-commit config so we can get these issues flagged in CI in the future? koalaman/shellcheck#pre-commit

I don't know if shellcheck can parse workflow files but actionlint has a feature where it runs shellcheck on the shell scripts in a workflow files. I think it's worth adding it for that specific feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release Related to the release process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants