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

Fixes #11601: Add a variable_string_from_command method #645

Conversation

amousset
Copy link
Member

@amousset
Copy link
Member Author

Do not merge for now, some small things to fix.

@amousset amousset force-pushed the ust_11601/add_a_variable_string_from_command_method branch from d40dd18 to e27c940 Compare October 17, 2017 16:00
@ncharles
Copy link
Member

this looks ok, but it is scary
what is the small thing to fix ?

@amousset
Copy link
Member Author

Commit modified

@amousset amousset force-pushed the ust_11601/add_a_variable_string_from_command_method branch from e27c940 to 2b99c52 Compare October 18, 2017 22:18
@amousset
Copy link
Member Author

Executing commands without the full path failed because execresult though the $() was an undefined variable, because it is evaluated after replacing const.dollar.

I move the /bin/true call which was not actually necessary at the beginning inside the command, and added a specific test case.

#
# We concatenate the return code as a three char string, and extract it after.
#
# /bin/true is necessary because execresult thinks the $() is an unexpanded variable.
Copy link
Member

Choose a reason for hiding this comment

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

can you update the doc to explain why the /bin/true is in the ${ } ?

@peckpeck
Copy link
Member

Looks good to me

@amousset
Copy link
Member Author

Commit modified

@amousset amousset force-pushed the ust_11601/add_a_variable_string_from_command_method branch from 2b99c52 to e9c84e6 Compare October 19, 2017 08:36
@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit e9c84e6 into Normation:v1.1 Oct 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants