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

Refs #4343 - Change behavior of service_check_running_ps in order to create all success classes if process match otherwise to create all errors classes #30

Merged
merged 2 commits into from
Jan 12, 2014

Conversation

nperron
Copy link
Contributor

@nperron nperron commented Jan 11, 2014

Refs #4343 - Change behavior of service_check_running_ps in order to create all success classes if process match otherwise to create all errors classes

cf http://www.rudder-project.org/redmine/issues/4343

@nperron
Copy link
Contributor Author

nperron commented Jan 11, 2014

This PR is not mergeable since a bug occurs in _classes_success and _classes_failure

@nperron
Copy link
Contributor Author

nperron commented Jan 11, 2014

Should be mergeable if #31 is merged before


"${service_regex}"
restart_class => "${class_prefix}_repaired";
restart_class => "${service_regex}_checked_not_ok";
Copy link
Member

Choose a reason for hiding this comment

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

In the two above class names, you don't use the ${class_prefix}. A simple convention in ncf is that all classes created must use the ${class_prefix}, to simplify namespace management and avoid conflicts.

Could you re-add the ${class_prefix} in? I don't see that this should cause any trouble, so long as you don't use a class name that might interfere with the result classes.

@jooooooon
Copy link
Member

Aside from the two issues I have mentioned (class_prefix and comments), this looks fine. Thanks for another quality PR !

Nicolas Perron added 2 commits January 12, 2014 22:03
@nperron
Copy link
Contributor Author

nperron commented Jan 12, 2014

I didn't see any specific reference of service_check_running_ps excepte from the tests of service_check_running and I have just updated them.

Is it OK for you ?

@jooooooon
Copy link
Member

Yes, thanks for the quick update! Merging.

jooooooon added a commit that referenced this pull request Jan 12, 2014
…ice_check_running

Refs #4343 - Change behavior of service_check_running_ps in order to create all success classes if process match otherwise to create all errors classes
@jooooooon jooooooon merged commit d574667 into Normation:master Jan 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants