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

Label name check for 'count_values' #4585

Merged
merged 1 commit into from Sep 13, 2018

Conversation

Projects
None yet
3 participants
@codesome
Copy link
Member

codesome commented Sep 7, 2018

Fixes #4584

Signed-off-by: Ganesh Vernekar cs15btech11018@iith.ac.in

@brian-brazil
Copy link
Member

brian-brazil left a comment

This looks good, can you add a unittest?

@codesome codesome force-pushed the codesome:invalid-label-name branch Sep 7, 2018

@codesome

This comment has been minimized.

Copy link
Member Author

codesome commented Sep 7, 2018

@brian-brazil I added a unit test. Would this be fine?

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Sep 7, 2018

That's fine thanks.

promql/engine_test.go Outdated
@@ -340,6 +345,12 @@ load 10s
t.Fatalf("unexpected error creating query: %q", err)
}
res := qry.Exec(test.Context())
if c.ShouldError {
if res.Err == nil {

This comment has been minimized.

This comment has been minimized.

Copy link
@codesome

codesome Sep 7, 2018

Author Member

Done

@codesome codesome force-pushed the codesome:invalid-label-name branch Sep 7, 2018

@gouthamve

This comment has been minimized.

Copy link
Member

gouthamve commented Sep 7, 2018

👍

promql/engine.go Outdated
@@ -1494,6 +1495,9 @@ func (ev *evaluator) aggregation(op ItemType, grouping []string, without bool, p
var valueLabel string
if op == itemCountValues {
valueLabel = param.(string)
if !model.LabelName(valueLabel).IsValid() {
ev.errorf("Invalid Label Name '%s'", valueLabel)

This comment has been minimized.

Copy link
@gouthamve

gouthamve Sep 12, 2018

Member

Taking another look, we use %q in the other parts of the code-base for quoting strings. Also, I think it should be all small letters, no capitals. Thoughts @brian-brazil?

This comment has been minimized.

Copy link
@brian-brazil

brian-brazil Sep 12, 2018

Member

@juliusv was looking at that recently. They should probably all be lower case.

This comment has been minimized.

Copy link
@codesome

codesome Sep 12, 2018

Author Member

@gouthamve I made the changes.

@codesome codesome force-pushed the codesome:invalid-label-name branch Sep 12, 2018

promql/engine.go Outdated
@@ -1494,6 +1495,9 @@ func (ev *evaluator) aggregation(op ItemType, grouping []string, without bool, p
var valueLabel string
if op == itemCountValues {
valueLabel = param.(string)
if !model.LabelName(valueLabel).IsValid() {
ev.errorf("invalid label name '%q'", valueLabel)

This comment has been minimized.

Copy link
@gouthamve

gouthamve Sep 12, 2018

Member

You don't need the '' anymore.

This comment has been minimized.

Copy link
@codesome

codesome Sep 12, 2018

Author Member

Oh, now I get the use of %q. Changed.

@codesome codesome force-pushed the codesome:invalid-label-name branch Sep 12, 2018

promql/engine_test.go Outdated
@@ -340,6 +346,10 @@ load 10s
t.Fatalf("unexpected error creating query: %q", err)
}
res := qry.Exec(test.Context())
if c.ShouldError {
testutil.NotOk(t, res.Err, "expected error for the query '%s'", c.Query)

This comment has been minimized.

Copy link
@gouthamve

gouthamve Sep 12, 2018

Member

Same here, sorry :/

This comment has been minimized.

Copy link
@codesome

codesome Sep 12, 2018

Author Member

Haha, done!

Label name check for 'count_values'
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>

@codesome codesome force-pushed the codesome:invalid-label-name branch to 7498dc1 Sep 12, 2018

@gouthamve gouthamve merged commit 576ee4d into prometheus:master Sep 13, 2018

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@codesome codesome deleted the codesome:invalid-label-name branch Sep 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.