-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 support for custom validations in promlint #1311
Add support for custom validations in promlint #1311
Conversation
Signed-off-by: João Vilaça <jvilaca@redhat.com>
Signed-off-by: João Vilaça <jvilaca@redhat.com>
e6697f3
to
83b0350
Compare
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.
Thanks!
I like this idea, but wonder if we have to break compatibility here. If we can let's avoid it - once this is sorted will review closer (:
prometheus/testutil/lint.go
Outdated
) | ||
|
||
// CollectAndLint registers the provided Collector with a newly created pedantic | ||
// Registry. It then calls GatherAndLint with that Registry and with the | ||
// provided metricNames. | ||
func CollectAndLint(c prometheus.Collector, metricNames ...string) ([]promlint.Problem, error) { | ||
func CollectAndLint(c prometheus.Collector, metricNames ...string) ([]validations.Problem, error) { |
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.
Can we avoid this?
client_golang is 1.0 and generally we can't break compatibility.
testutil might be experimental (?) - not sure if we ever mentioned it's compatibility level, but if we can-- let's not break the compatibility here. Let's move Problem back to promlint and work from there (perhaps that might require validations to be promlint package).
WDYT?
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.
I understand your concerns, I will update it and see if I can get a clean solution
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.
@bwplotka updated
Signed-off-by: João Vilaça <jvilaca@redhat.com>
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.
Thanks! Epic 💪🏽
for i, t := range dto.MetricType_name { | ||
if i == int32(dto.MetricType_UNTYPED) { | ||
continue | ||
for _, fn := range defaultValidations { |
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, why not merging those slices (default and custom) and not repeating the code?
Not a blocker, that's the only nit I would improve, merging anyway.
Thanks! I would also recommend adding useful validators to our repo (: |
No description provided.