-
Notifications
You must be signed in to change notification settings - Fork 19
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
WiP: Clean elements groups and refactor configuration warnings/errors logging #331
Conversation
mohierf
commented
Sep 12, 2016
•
edited
Loading
edited
- replace groups members property from String to List, this to avoid lists with one empty string element
- fixes Hostgroup does not store its hostgroups_members #93
- fixes Add property hostgroup_members in Hostgroup class #110
- refactor unit tests for groups
- refactor configuration warnings/errors logging
Remove old test test_groups_pickle
Remove old test testgroups_with_no_alias
…guration failed message
…. Each object declaring special properties will assume their check
if self.configuration_warnings: | ||
logger.info("Configuration warnings:") | ||
for msg in self.configuration_warnings: | ||
logger.info(msg) |
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.
for warning and errors, use logger.warning instead logger.info
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.
No, bad idea. As explained in the comment, using INFO is better beceause these logs are simply used to build a sum-up when configuration check is finished. If we use WARNING and ERROR, each warning and error will appera twice in the logs ... bad idea if we want to parse the logs with an application like LogStash :/
@@ -301,6 +301,14 @@ def get_name(self): | |||
return self.name | |||
return 'SERVICE-DESCRIPTION-MISSING' | |||
|
|||
def get_servicegroups(self): |
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.
We can remove this function by replace get_servicegroups() calls by self.servicegroups
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.
Yes, why not ... but there is so much functions like this one. We should organize another code cleaning session to fix all this kind of functions later.
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.
okay