-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Make service check statuses available as constants #2960
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2960 +/- ##
=========================================
- Coverage 84.86% 84.56% -0.3%
=========================================
Files 652 663 +11
Lines 36125 38139 +2014
Branches 4292 4559 +267
=========================================
+ Hits 30656 32253 +1597
- Misses 4203 4538 +335
- Partials 1266 1348 +82 |
@@ -28,6 +28,7 @@ | |||
using_stub_aggregator = True | |||
|
|||
from ..config import is_affirmative | |||
from ..constants import ServiceCheck |
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.
why move this to a new file?
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.
Well it's a constant so it makes sense. Also there are others below like ONE_PER_CONTEXT_METRIC_TYPES
that we should eventually move there as well
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.
Being able to access them from base seems very handy.
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.
Yea, that makes sense. But since these all still need to be members of the class it would just cut down on duplication in the AgentCheck classes, I suppose
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.
Looks good to me. Thanks
Can you elaborate on what change you're planning on making that will rely upon this? I don't dislike this, I just wanna know the context |
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.
Actually I think @gmmeyer comment is very relevant here.
|
Is there a check you want to use this in now? |
Yes, |
Motivation
Easier access for utility functions that map product statuses to service check statuses