[prebuilds] Introduce 'failed' state for prebuilds #8601
Conversation
Distinguish between failed tasks but finished prebuilds and fully failed prebuilds (no snapshot) fixes #8592
| return (<span className="font-medium text-blue-500 uppercase">running</span>); | ||
| case "aborted": | ||
| return (<span className="font-medium text-gray-500 uppercase">canceled</span>); | ||
| case "failed": |
There was a problem hiding this comment.
Drive-by comment: You probably want to handle "failed" in prebuildStatusIcon below as well (there is already a StatusFailed icon you can re-use).
There was a problem hiding this comment.
As the number of error cases grows, it becomes harder to enumerate them. One possible way to simplify the API UX is to have a single error state and extend it with a cause which is set to a computer readable reason for the failure - TimedOut, RateLimited, FailedToStart, etc. The error message then becomes a human readable string to show in the UI etc.
There was a problem hiding this comment.
Good point @easyCZ. Alternatively, I think we could also special-case the few non-error cases, then consider "all the rest" as errors?
There was a problem hiding this comment.
Depends how far we want to go. Some failures are due to user issues, some can be due to system issues. I think the it mostly depends if we control all the clients which will be interpreting the status (Dashboard, etc) or if some of the clients are/will be out of our control (implementing against an API). In the former case, it doesn't matter much as we can largely reach a shared understanding of what is an error and what isn't. But if an external frontend/server needs to switch on these, it may be less obivous to them with a large number of cases.
I think this is probs not something we need/should decide now and I don't think it blocks the change.
| prebuild.snapshot = status.conditions!.snapshot; | ||
| headlessUpdateType = HeadlessWorkspaceEventType.FinishedButFailed; | ||
| } else if (!status.conditions!.snapshot) { | ||
| prebuild.state = "failed"; |
There was a problem hiding this comment.
Suggestion: Here you could also set prebuild.error to something like "Prebuild does not have a snapshot".
There was a problem hiding this comment.
That is a symptom not the actual error. I've discussed with Gero that we are also going to not overwrite actual errors if they exist (in case of multiple stop messages).
|
wow, that was quick feedback 😁 The branchname was too long so I had to create another PR. |
5 participants
Description
This PR introduces a "failure" state for prebuilds that have entirely failed to finish.
This is to distinguish them from failed tasks but finished rebuilds.
Related Issue(s)
fixes #8592
Release Notes