-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add alerting manager, definations and generic exceptions #22
Conversation
@Tendrl/tendrl-core This is a split PR of #15 Please review |
@@ -0,0 +1,47 @@ | |||
# flake8: noqa |
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.
Name of the file should be something like tendrl_definitions_alerting.py.
Also the package name followed in other cases are like tendrl/{module}/manager. Can we think of something similar?
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.
ok
type: json | ||
Alert: | ||
attrs: | ||
alert_id: |
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.
Add help
attribute for all
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.
ok
from tendrl.common import log | ||
|
||
|
||
LOG = logging.getLogger(__name__) |
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.
Refactor as per new changes
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.
Ok
config.get('alerting', 'log_cfg_path'), | ||
config.get('alerting', 'log_level') | ||
) | ||
manager = Manager() |
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.
The name of this class could be AlertingManager and it should extend from common's Manager class.
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.
common's manager cannot be extended here because, it has certain things like the discovery_thread among others which are not used anywhere in monitoring as of now. Also this manager needs to be a multiprocessing.Process as it has blocking calls in some of the managers managed by this manager
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.
Renaming -- ok
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.
LGTM
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.
Fix travis unit test failures
@@ -0,0 +1,75 @@ | |||
import logging |
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.
Need to remove this file as its no more required.
tendrl-bug-id: #10,#11,#12,#14
Signed-off-by: anmolbabu anmolbudugutta@gmail.com