-
Notifications
You must be signed in to change notification settings - Fork 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
Handle unit length valueFrom
values
#1958
Conversation
0edaa54
to
3091b3c
Compare
Handle unit length `valueFrom` values and skip strings where we expect them to be type-cast-able to `float64`, instead of erroring, since that is the expected behavior, and what's being done for other types.
/triage accepted |
@@ -173,6 +173,17 @@ func Test_values(t *testing.T) { | |||
newEachValue(t, 2, "type", "type-a", "active", "1"), | |||
newEachValue(t, 4, "type", "type-b", "active", "3"), | |||
}}, | |||
{name: "path-relative valueFrom value", each: &compiledGauge{ |
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 add another Test for this path
// This means we are here: [ <string>, <int>, ... ] (eg., [ "foo", "0", ... ], i.e., <path>.foo[0]...
// ^
// Skip over.
return nil```
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.
This is actually another bug in and of itself. Basically, the same one but for multi-length valueFrom
paths. In the spirit of keeping the changesets digestible and throughly tested and reviewed in the CRS realm, I'm thinking if it'd be a good idea to target that in another PR (and getting the unit-length path fix in this release)?
JFYI the test below returns nil
for both main
and this PR.
{
name: "multiple path-relative valueFrom values", each: &compiledGauge{
compiledCommon: compiledCommon{
path: mustCompilePath(t, "spec"),
labelFromPath: map[string]valuePath{
"version": mustCompilePath(t, "version"),
},
},
ValueFrom: mustCompilePath(t, "order", "-1", "id"),
}, wantResult: []eachValue{
newEachValue(t, 3, "version", "v0.0.0"),
},
},
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.
Sounds good to me! Can you add a TODO to the code path?
sValueFrom := c.ValueFrom.String() | ||
// No comma means we're looking at a unit-length path (in an array). | ||
if !strings.Contains(sValueFrom, ",") && | ||
sValueFrom[0] == '[' && |
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 add another check for sValueFrom[len(sValueFrom)-1] == ']'
?
I can confirm your change is fixing #1947. I would like to get a second pair of eyes on the implementation from @kubernetes/kube-state-metrics-maintainers |
/assign I asked some questions in #1947. It is unclear what we are trying to achieve here. |
6beb19f
to
39efcf5
Compare
sValueFrom := c.ValueFrom.String() | ||
// No comma means we're looking at a unit-length path (in an array). | ||
if !strings.Contains(sValueFrom, ",") && | ||
sValueFrom[0] == '[' && sValueFrom[len(sValueFrom)-1] == ']' && | ||
// "[...]" and not "[]". | ||
len(sValueFrom) > 2 { | ||
extractedValueFrom := sValueFrom[1 : len(sValueFrom)-1] |
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.
This is a bit odd to me. ValueFrom is orginally a string array and here we are manually trying to parse it as a string and extracting the first element from it, when we should have an easy access to it. What do you think about adding a new method to valuePath
that would return a string array instead of a raw string?
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.
@rexagod I am merging the PR without that change since it is mostly code quality related, but if my suggestion makes sense to you, let's address that in a follow-up
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgrisonnet, mrueg, rexagod The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it: Handle unit length
valueFrom
values and skip strings where we expect them to be type-cast-able tofloat64
, instead of erroring, since that is the expected behavior, and what's being done for other types.How does this change affect the cardinality of KSM: No change.
Which issue(s) this PR fixes: Fixes #1947