Repository navigation
apis.errs.Also does not propagate error level #2514
Description
/area API
/kind bug
Steps to Reproduce the Problem
Using "warning" level error introduced in #2498:
func TestFoo(t *testing.T) {
var errs *apis.FieldError
errs = errs.Also(apis.FieldError("foo").At(apis.WarningLevel))
if errs.Level != apis.WarningLevel {
t.Errorf("Expected an error at warning level but got level %s", errs.Level)
}
}
Expected Behavior
The aggregated error has the level that is the highest level of any errors combined with Also.
In the example above, I would expect errs.Level to be Warning, and errs.Filter(apis.WarningLevel).Level to also be Warning.
Actual Behavior
In the example above, errs.Level and errs.Filter(apis.WarningLevel).Level are both Error. (i.e. an error at level "warning" aggregated with no error turns into an error at level "error")
Activity
mattmoor commentedon May 5, 2022
Thanks for opening this 🙏
cc @dprotaso @evankanderson @vaikas @n3wscott
To reiterate some of the thread in Tekton, this is sort of a reflection of the awkward conflation of
apis.FieldErroras both an error and an aggregation of errors (via.Also())In your example,
errs.Messagewon't match the single error it contains either, so I think the breakdown here is more general than the newLevelfield. I wonder if for aggregated errors we should setLevelto reflect the constituents and perhaps add aMixedlevel when multiple levels are present? This would at least make the cases @lbernick describes a bit more intuitive. I'd guess it would avoid back-compat problems since nothing is really using anything but error yet, so nothing should get a mixed level with.Also.What do folks think?
vaikas commentedon May 5, 2022
@mattmoor you have a link to Tekton thread handy since it seems to have more context?
mattmoor commentedon May 5, 2022
https://tektoncd.slack.com/archives/CLCCEBUMU/p1650971358662559
evankanderson commentedon May 5, 2022
I'd expect the "lowest" level when aggregating via
Also(sinceErrorLevelis 0 and counts upward), but otherwise I agree with the diagnosis.Given that this code is unlikely to be in a tight loop, I wonder if looping over the set of contained errors to determine the correct collection-level would be correct. You could early-exit the loop if
ErrorLevelis encountered, since that's the lowest level. If we imagine aSuggestionlevel that's less severe thanWarning:Alsoed (symmetrical)The worst case performance would be constructing a long chain of
Warninglevel errors, where you'd end up scanning O(n^2) errors as you add one to the head of the chain, but this is probably acceptable (and maybe we'd cache things so it's actually O(n), I haven't thought too deeply).github-actions commentedon Aug 4, 2022
This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with
/reopen. Mark the issue asfresh by adding the comment
/remove-lifecycle stale.dprotaso commentedon Aug 5, 2022
/lifecycle frozen
JeromeJu commentedon Sep 14, 2022
Thanks @evankanderson for the table of the combinations of errs incurred.
I would like to confirm my understanding on the O(n) solution, would that need to introduce a hash-table in a (possible cyclic?) graph?
evankanderson commentedon Sep 15, 2022
You shouldn't need a hash table for caching, as you only need to track the "head" of the chain as long as the cache is correct. Using
.Also()in any sort of correct issue should produce a tree, not an arbitrary graph.If you want, you could use the go benchmark library to compare approaches if you want to learn that...