Double quote escape for bash script ENV_VAR part #5349
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## develop #5349 +/- ##
===========================================
+ Coverage 82.13% 82.13% +0.01%
===========================================
Files 533 533
Lines 38485 38491 +6
===========================================
+ Hits 31605 31611 +6
Misses 6880 6880
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
sphuber
requested changes
Feb 12, 2022
tests/common/test_escaping.py
Outdated
Comment on lines
33
to
50
| def test_escape_for_bash_double_quotes(): | ||
| """ | ||
| Tests various inputs for `aiida.common.escaping.escape_for_bash`. | ||
| double quotes tests especially useful for ENV variables with $ symbol | ||
| """ | ||
|
|
||
| tests = ( | ||
| [None, ''], | ||
| ['string', '"string"'], | ||
| ['string with space', '"string with space"'], | ||
| ['string with a " double quote', '''"string with a "'"'" double quote"'''], | ||
| [1, '"1"'], | ||
| [2.0, '"2.0"'], | ||
| ['$PWD', '"$PWD"'], | ||
| ) | ||
|
|
||
| for string_input, string_escaped in tests: | ||
| assert escape_for_bash(string_input, use_double_quotes=True) == string_escaped |
There was a problem hiding this comment.
This is the perfect candidate to use `pytest.mark.parametrize. I would suggest replacing the original test and the one you added with the following single test:
Suggested change
| def test_escape_for_bash_double_quotes(): | |
| """ | |
| Tests various inputs for `aiida.common.escaping.escape_for_bash`. | |
| double quotes tests especially useful for ENV variables with $ symbol | |
| """ | |
| tests = ( | |
| [None, ''], | |
| ['string', '"string"'], | |
| ['string with space', '"string with space"'], | |
| ['string with a " double quote', '''"string with a "'"'" double quote"'''], | |
| [1, '"1"'], | |
| [2.0, '"2.0"'], | |
| ['$PWD', '"$PWD"'], | |
| ) | |
| for string_input, string_escaped in tests: | |
| assert escape_for_bash(string_input, use_double_quotes=True) == string_escaped | |
| import pytest | |
| from aiida.common.escaping import escape_for_bash | |
| @pytest.mark.parametrize(('to_escape, expected_single_quotes, expected_double_quotes'), ( | |
| (None, ''), | |
| ('string', "'string'", '"string"'), | |
| ('string with space', "'string with space'", '"string with space"'), | |
| ("string with ' single quote", """'string with '"'"' single quote'""", '''"string with "'"'" double quote"'''), | |
| (1, "'1'", '"1"'), | |
| (2.0, "'2.0'", '"2.0"'), | |
| ('$PWD', "'$PWD'", '"$PWD"'), | |
| )) | |
| def test_escape_for_bash(to_escape, expected_single_quotes, expected_double_quotes): | |
| """Tests various inputs for `aiida.common.escaping.escape_for_bash`.""" | |
| assert escape_for_bash(to_escape, use_double_quotes=False) == expected_single_quotes | |
| assert escape_for_bash(to_escape, use_double_quotes=True) == expected_double_quotes |
There was a problem hiding this comment.
Thanks @sphuber! Absolutely it is beautiful. One line to rule all test cases 😄
I also change the name to environment_variables_double_quotes as you suggest.
unkcpz
force-pushed
the
fix/4836/double-quotes-for-envvar
branch
3 times, most recently
from
ec6ab39 to
3c67e78
Compare
sphuber
reviewed
Feb 13, 2022
| @@ -248,6 +248,9 @@ class JobTemplate(DefaultFieldsAttributeDict): # pylint: disable=too-many-insta | |||
| * ``rerunnable``: if the job is rerunnable (boolean) | |||
| * ``job_environment``: a dictionary with environment variables to set | |||
| before the execution of the code. | |||
| * ``environment_variables_double_quotes``: if set to True, use double quotes | |||
| instead of single quotes to escape the environment variables specified | |||
| in ``environment_variables``. | |||
There was a problem hiding this comment.
Suggested change
| in ``environment_variables``. | |
| in ``job_environment``. |
There was a problem hiding this comment.
@unkcpz just noticed this last mistake in the docstring. Sorry for having missed it. If you correct this, I will merge the PR. Thanks a lot
There was a problem hiding this comment.
@sphuber Thanks. Updated.
unkcpz
force-pushed
the
fix/4836/double-quotes-for-envvar
branch
2 times, most recently
from
35d47b7 to
5ae6d28
Compare
The escape method will escape the string in the quotes. The defalut implementation is using single quotes to escape the string. However, single quotes will not eval the spacial shell parameters such as $ and @, this makes bash script not versatile to set the parameters in runtime. The `use_double_quotes` option is added to escape the string in the double quotes so the spacial parameters are still valid in the shell script. Using double escape option into environment variables of job templete setting address aiidateam#4836. User can now use double quotes escaping in calcjob option to escape the environment variables of scheduler.
unkcpz
force-pushed
the
fix/4836/double-quotes-for-envvar
branch
from
8732249 to
d79ebff
Compare
for more information, see https://pre-commit.ci
sphuber
approved these changes
Feb 13, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is separated from #5280 and containerized code PR for easy review.
Fixes #4836