mortent
commented
2 years ago I think using the existing Error fields that we already have on most of the events would be the best option here. It aligns well with the existing semantics where errors communicated through the Apply|Prune|Delete|ValidationEvents are considered non-fatal, while the ErrorEvent is used only when we encounter errors that are fatal and the operation are aborted. Using errors also allows to use typed errors, so it can make it easier for users to understand the cause without resorting to string matching.
I think an somewhat open question is whether we should distinguish between skip "errors" and other errors? Currently we don't provide a Apply|Prune|DeleteError operation, as users should check for errors by looking at the Errors property. The reasons for this design was that we didn't want to allow for situations where the operation were something else than Apply|Prune|DeleteError, but still had a non-nil Error. It seems to be that we probably want to have separate operation values for skipped vs other errors.
k8s-triage-robot
karlkfi
There are now multiple filters that can cause an ApplyTask or DeleteTask to be skipped. Currently, the reason returned by the filter is being logged but not exposed to the caller. This makes it hard to understand why an object's actuation was skipped.
Filters:
deletion: detachoron-remove: keepannotationAll of these filters should probably inform the user why the object was skipped.
However, since some callers might need to detect certain reasons, we can't just return the reason as a string. It needs to be a typed object.
And since we made all the apply/prune/wait tasks "continue on error", we can just send typed errors with the skip operation event, instead of adding a dedicated reason string field.