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

Remove quotes in the argument of python /app/print-hash.py #91

Merged
merged 1 commit into from Jul 25, 2022
Merged

Remove quotes in the argument of python /app/print-hash.py #91

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion twine-upload.sh
View file
Open in desktop
Expand Up @@ -45,7 +45,7 @@ if [[ ${INPUT_VERBOSE,,} != "false" ]] ; then
fi

if [[ ${INPUT_PRINT_HASH,,} != "false" || ${INPUT_VERBOSE,,} != "false" ]] ; then
python /app/print-hash.py "${INPUT_PACKAGES_DIR%%/}"
python /app/print-hash.py ${INPUT_PACKAGES_DIR%%/}
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this patch may end up opening a can of worms because unquoted variable interpolation may turn into multiple arguments if it has spaces in it.
Have you checked such problematic scenarios? I'd very much like to be more confident with this...
Also, why not change this script to work the same way as other commands accepting ${INPUT_PACKAGES_DIR%%/}/*?
Any ideas?

Copy link
Member Author

@dukecat0 dukecat0 Jul 25, 2022 •
edited

Choose a reason for hiding this comment

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

I just had a few tests on it.

For example, a path like /github/workspace/dist package/ may fail all twine commands since there is a space in the path.
Also, error like this will be displayed:

Warning:  It looks like there are no Python distribution packages to publish in the directory 'dist package/'. Please verify that they are in place should you face this problem.

Because these arguments are not quoted:

gh-action-pypi-publish/twine-upload.sh

Lines 21 to 22 in 1bbe3c9

if ( ! ls -A ${INPUT_PACKAGES_DIR%%/}/*.tar.gz &> /dev/null && \
! ls -A ${INPUT_PACKAGES_DIR%%/}/*.whl &> /dev/null )

gh-action-pypi-publish/twine-upload.sh

Line 35 in 1bbe3c9

twine check ${INPUT_PACKAGES_DIR%%/}/*

gh-action-pypi-publish/twine-upload.sh

Line 54 in 1bbe3c9

exec twine upload ${TWINE_EXTRA_ARGS} ${INPUT_PACKAGES_DIR%%/}/*

Copy link
Member Author

@dukecat0 dukecat0 Jul 25, 2022 •
edited

Choose a reason for hiding this comment

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

Also, why not change this script to work the same way as other commands accepting ${INPUT_PACKAGES_DIR%%/}/*?

Because iterdir() is used in the script so using ${INPUT_PACKAGES_DIR%%/}/* will result in NotADirectoryError.

Copy link
Member

Choose a reason for hiding this comment

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

I meant that you wouldn't need to use iterdir() because the outer shell would resolve that into multiple arguments IIUC.

fi

TWINE_USERNAME="$INPUT_USER" \
Expand Down