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 #10388: Add a generic method that handles using options in package actions #577

Conversation

amousset
Copy link
Member

@amousset
Copy link
Member Author

Commit modified

@amousset amousset force-pushed the ust_10388/add_a_generic_method_that_handles_using_options_in_package_actions branch from 2a99c9f to 8bf2abf Compare April 25, 2017 12:34
@amousset amousset force-pushed the ust_10388/add_a_generic_method_that_handles_using_options_in_package_actions branch from 8bf2abf to 2ab7304 Compare April 25, 2017 12:44
@amousset
Copy link
Member Author

Commit modified

Copy link
Member

@peckpeck peckpeck left a comment

Choose a reason for hiding this comment

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

Why do it on version 1.0

# name old version new version
# | | |
# /-------+-------\ /--+--\ /------+-------\
match = re.match("^Inst\s+(?P<name>[^\s:]+)(?::\S+)?\s+\[[^\]\s]+\]\s+\((?P<version>\S+)" +
match = re.match("^Inst\s+(?P<name>[^\s:]+)(?::\S+)?\s+\[[^]\s]+\]\s+\((?P<version>\S+)" +
Copy link
Member

Choose a reason for hiding this comment

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

removing this \ seems to be a mistake

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 comes from the CFEngine script we had modified a bit. It does not change the behavior (while a bit less readable).

# repository(ies) arch (might be optional)
# | |
# /--+-\ /---------+---------\
"\s+[^[)]*(\s\[(?P<arch>[^]\s]+)\])?\).*", line)
Copy link
Member

Choose a reason for hiding this comment

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

even if not mandatory, there should be a \ before the inside [

Copy link
Member Author

Choose a reason for hiding this comment

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

same here

@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit 2ab7304 into Normation:v1.0 May 19, 2017
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants