SHIP-0024: Surfacing Errors to BuildRuns #30
Conversation
| results. It requires iterating all steps and processing the message | ||
| field. |
There was a problem hiding this comment.
Not all steps but the first one that failed = the one that we store in status.failedAt.
There was a problem hiding this comment.
Good point. However it introduces a dependency that UpdateBuildRunUsingTaskRunCondition has to be run before the error extracting logic.
dalbar
force-pushed
the
0024-surfacing-errors-to-buildrun
branch
4 times, most recently
from
be2dac7 to
e349b62
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SaschaSchwarze0 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 |
openshift-ci
bot
added
the
approved
dalbar
force-pushed
the
0024-surfacing-errors-to-buildrun
branch
from
e349b62 to
86c09d9
Compare
|
Initial implementation for reference: shipwright-io/build#901 |
|
/assign coreydaley |
| This ship provides a mechanism to surface errors in the build process | ||
| into BuildRun objects granting more visibility into | ||
| without being Tekton/Implementation specific. To | ||
| achieve this goal, we introduce two new generic results | ||
| `shp-error-reason` and `shp-error-message`, that a TaskRun can emit | ||
| and a BuildRun can pick up. |
There was a problem hiding this comment.
I think that these should be more generic, which would allow for surfacing not only error messages and reasons, but success messages and reasons (and anything in between).
There was a problem hiding this comment.
As mentioned in shipwright-io/build#901 (comment). I would rather prefer a dedicated field for errors. Do you see a specific use-case where we would need a generic message/reason that is not fitting into our current build results and also not into the possible future implementation of the strategy author results (see https://github.com/shipwright-io/community/blob/main/ships/0023-surface-results-in-buildruns.md#strategy-results-api-proposal).
There was a problem hiding this comment.
So some analogous "prior art" that conforms with I think applies to what is laid out here:
-
we are really talking about "communicating" information discovered by one controller that is of interest to another controller, where some sort of dependency between the controllers exists
-
this is analogous in my opinion to what the openshift cluster version operator gathers from its dependent operators (where operator == controller + mgmt API)
-
and if you look at that contract, you'll see something very similar. Consider: https://github.com/openshift/api/blob/release-4.8/operator/v1/types.go#L158-L164 ... note
reasonanderror -
and more immediately / directly, look at what exists in shipwright that looks eerily similar: https://github.com/shipwright-io/build/blob/ac139dba9437bca165137856e18a593417945630/pkg/apis/build/v1alpha1/buildrun_types.go#L241-L261
So I see a successful pattern that has be reapplied a few times.
I would contend the focus on errors is a simplifying assumption that removes the need for type/status/last transition time from that operator type I noted above
the intermediate / non-terminal TaskRun statues are captured via other elements of the existing shipwright API
so in that sense, fwiw I at least am good with only introducing reason/message (again) in our API
that said, I do wonder about where in the build run status we could surface our failed reason/message
I'll see if it makes sense to dive into that as I progress with this
There was a problem hiding this comment.
general approval from me @dalbar
but I think there is one opportunity for making our status a bit more compact and organized
thanks
| This ship provides a mechanism to surface errors in the build process | ||
| into BuildRun objects granting more visibility into | ||
| without being Tekton/Implementation specific. To | ||
| achieve this goal, we introduce two new generic results | ||
| `shp-error-reason` and `shp-error-message`, that a TaskRun can emit | ||
| and a BuildRun can pick up. |
There was a problem hiding this comment.
So some analogous "prior art" that conforms with I think applies to what is laid out here:
-
we are really talking about "communicating" information discovered by one controller that is of interest to another controller, where some sort of dependency between the controllers exists
-
this is analogous in my opinion to what the openshift cluster version operator gathers from its dependent operators (where operator == controller + mgmt API)
-
and if you look at that contract, you'll see something very similar. Consider: https://github.com/openshift/api/blob/release-4.8/operator/v1/types.go#L158-L164 ... note
reasonanderror -
and more immediately / directly, look at what exists in shipwright that looks eerily similar: https://github.com/shipwright-io/build/blob/ac139dba9437bca165137856e18a593417945630/pkg/apis/build/v1alpha1/buildrun_types.go#L241-L261
So I see a successful pattern that has be reapplied a few times.
I would contend the focus on errors is a simplifying assumption that removes the need for type/status/last transition time from that operator type I noted above
the intermediate / non-terminal TaskRun statues are captured via other elements of the existing shipwright API
so in that sense, fwiw I at least am good with only introducing reason/message (again) in our API
that said, I do wonder about where in the build run status we could surface our failed reason/message
I'll see if it makes sense to dive into that as I progress with this
dalbar
force-pushed
the
0024-surfacing-errors-to-buildrun
branch
from
86c09d9 to
eaf6ab9
Compare
|
The Ship now includes #30 (comment) and more alternatives including conditions. |
dalbar
force-pushed
the
0024-surfacing-errors-to-buildrun
branch
2 times, most recently
from
1b0396e to
5e046a4
Compare
|
LGTM I'll defer putting the actual label for now in case @SaschaSchwarze0 wants one more pass before this merges thanks @dalbar ! |
Added initial draft for error surfacing ship. Update ships/0024-surfacing-errors-to-buildrun.md Co-authored-by: Sascha Schwarze <[email protected]>
dalbar
force-pushed
the
0024-surfacing-errors-to-buildrun
branch
from
5e046a4 to
c063203
Compare
/lgtm |
This ship proposes introducing error messages from low-level resources like TaskRuns into BuildRuns. So that users can better understand the underlying issues and third-party applications have an API to consume them.