-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Remove quotes in the argument of python /app/print-hash.py
#91
Conversation
python /app/print-hash.py
@@ -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%%/} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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%%/}/* |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at #90 (comment), I think I'll merge this now. But for the future, it may be worth looking into improving the quoting.
Fix #90