[dashboard] Show prebuild status based on prebuild workspace record #8805

easyCZ · 2022-03-14T16:20:47Z

Description

Currently, the Prebuild detail view (with logs and status) uses the WorkspaceInstance status. This is inconsistent with respect to the Prebuild List view. This PR uses the prebuild status instead of status from the WorkspaceInstance.

This PR does not remove all instances of this inconcistent usage (another in ConfigureProject.tsx). It will be removed in a subsequent PR.

Related Issue(s)

Related to Some failed prebuilds use a wrong status label #5611
Relates to Some prebuilds use different status on project cards and prebuild page #7384
Relates to [dashboard, (server?)] Inconsistent derivation of prebuild status #8197

How to test

Preview env
Add/clone https://gitlab.com/gitpod-milan/gitpod-large-image as a Project
Trigger builds for the following branches:
- main
- pass
- timeout
- fail-prebuild
Observe various statuses reported
Trigger a new prebuild and cancel it manually - observe cancelled status

Release Notes

Dashboard reports prebuild status consistently between list view and detail view.

Documentation

How it looks

Open questions

Designs could be improved with respect to showing the underlying error. I think we can follow-up with a cleanup of the styling to communicate the underlying error message more cleanly

/hold

/werft with-clean-slate-deployment

easyCZ · 2022-03-16T12:08:24Z

/werft run

👍 started the job as gitpod-build-mp-pb-status.7

easyCZ · 2022-03-16T12:12:01Z

@gtsiolis The current way we report the underlying error (next to the status) is a bit ugly. Do you have thoughts on what we could do here? For now, we only have a string as the error but I'm working on making this a bit richer as well.

Co-related: The statuses we have here don't always convey well. For example, we show Failed in red, but it was in a way a "user error" - something defined by the prebuild tasks didn't complete succesfully. I think here it would helpful to better convey that the system is fine, but the task is not.

gtsiolis · 2022-03-16T12:19:05Z

ETA: Adding this to scheduled to take a look later today or tomorrow.

Thanks for the ping @easyCZ! 🏓

andrew-farries · 2022-03-16T17:36:16Z

Gave this a quick look just now and after triggering prebuilds on all branches, they all failed with:

easyCZ · 2022-03-16T17:51:11Z

Ah, good old staging not working again. Will take a look either later or next week (hols Thu /Fri)

easyCZ · 2022-03-21T10:28:43Z

@andrew-farries I've rebased and re-deployed this branch, I've also run the above tests and I've been able to reproduce the desired changes

.

AlexTugarev · 2022-03-29T05:54:15Z

/werft run

👍 started the job as gitpod-build-mp-pb-status.9

AlexTugarev · 2022-03-29T08:52:15Z

components/dashboard/src/projects/Prebuilds.tsx

+}
+
+// Deprecated. Use PrebuildStatus instead.
+export function PrebuildInstanceStatus(props: { prebuildInstance?: WorkspaceInstance; prebuild?: PrebuildWithStatus }) {


I see, it's still used in ConfigureProject component. OK

AlexTugarev

Works as advertised!

Thanks!

components/dashboard/src/projects/Prebuild.tsx

jankeromnes

This is fantastic, many thanks @easyCZ! 👏

Code changes also look good to me, and the PR seems to work as advertised. 🎯

@gtsiolis Would you like to review the "How it looks" screenshots in the PR description? 😁 (Personally, this looks good to me and is already better than what we have today in production, so we could also merge this and make any adjustments in follow-ups. 💭)

Co-authored-by: Jan Keromnes <[email protected]>

jankeromnes

Re-approving post nit-fix ✅

easyCZ · 2022-03-29T09:24:22Z

/unhold

gtsiolis · 2022-03-29T09:25:07Z

Taking a look now! 👀

But feel free to merge this as is.

gtsiolis

I'm late here but left some comments we could have addressed in this iteration. Posted here in case we'd like to open follow up issues or PRs for these.

Cc @andrew-farries @jankeromnes @AlexTugarev @easyCZ

gtsiolis · 2022-03-29T10:27:28Z

components/dashboard/src/projects/Prebuilds.tsx

+            if (props.prebuild?.error) {
+                return (
+                    <span>
+                        The tasks executed in the prebuild returned a non-zero exit code. {props.prebuild.error}


question: This could be confusing. Could we use simpler words to describe the error. Maybe something like the following?

A task configured to run in the prebuild returned a non-zero exit code.

gtsiolis · 2022-03-29T10:29:05Z

components/dashboard/src/projects/Prebuilds.tsx

+            if (props.prebuild?.error) {
+                return (
+                    <span>
+                        The tasks executed in the prebuild returned a non-zero exit code. {props.prebuild.error}


suggestion: Also, the error appended there is already included in the log output above, right? If yes, I think we could safely drop it from there. WDYT?

Definitely. I'm trying to clean it up incrementally as there's a lot of tech debt with respect where we collect the "truth" from. Unify first, then improve the messaging.

I'll cut a ticket for this concrete case to not show the same error twice.

Unify first, then improve the messaging.

💯

I'll cut a ticket for this concrete case to not show the same error twice.

Thanks, @easyCZ! 🏀

fyi: I've opened #9111 to keep track of this discussion and the unresolved comments below.

gtsiolis · 2022-03-29T10:30:48Z

components/dashboard/src/projects/Prebuilds.tsx

+                    Prebuild has been cancelled. Either a user cancelled it, or the prebuild rate limit has been
+                    exceeded. {props.prebuild.error}


suggestion: Again, a simpler message here could help while dropping appending information that is already duplicated. WDYT?

Prebuild has been manually cancelled or prebuild rate limit has been reached.

gtsiolis · 2022-03-29T10:32:15Z

components/dashboard/src/projects/Prebuilds.tsx

+function PrebuildStatusDescription(props: { prebuild: PrebuildWithStatus }) {
+    switch (props.prebuild.status) {
+        case "queued":
+            return <span>Prebuild is queued and will be processed when there is execution capacity.</span>;


thought: Sounds accurate but a bit scary for humans, no? Maybe we could use something like the following:

Prebuild is queued and will be processed in a moment.

gtsiolis · 2022-03-29T10:33:37Z

components/dashboard/src/projects/Prebuilds.tsx

+        case "timeout":
+            return (
+                <span>
+                    Prebuild timed out. Either the image, or the prebuild tasks took too long. {props.prebuild.error}


suggestion: Maybe the following suffices?

Prebuild timed out because the image or prebuild tasks took too long.

Optiontally, we could append some information about the max prebuild limit.

Prebuild timed out because the image or prebuild tasks took too long. Maximum prebuild duration is 1 hour.

easyCZ · 2022-03-29T10:47:48Z

I'm late here but left some comments we could have addressed in this iteration. Posted here in case we'd like to open follow up issues or PRs for these.

Cc @andrew-farries @jankeromnes @AlexTugarev @easyCZ

Thanks for the feedback. That makes sense. From my perspective, I'd be interested in exposing the full error message but possibly it can be inside (by default) collapsed component. Surfacing the information does allow us to get better feedback on the errors from users and be able to address them. WDYT @gtsiolis?

My intention for follow-up PRs is to clean-up the errors the server provides such that we treat them as "publicly facing errors" and include actionable feedback.

Optiontally, we could append some information about the max prebuild limit.

On the topic of these, the error message should really come directly from the backend. We're entering a world where workspace features can configurable and in that case only the backend will be authoritative over what the timeout (or other properties) actually are. I think we'll see us move away more from the UI having the error text and more it being sourced directly from the DB as a user facing error.

gtsiolis · 2022-03-29T10:57:04Z

From my perspective, I'd be interested in exposing the full error message but possibly it can be inside (by default) collapsed component. Surfacing the information does allow us to get better feedback on the errors from users and be able to address them. WDYT @gtsiolis?

Fully agree on beng able to see the full error message. However, we can mask the form of the error on the elements that do not scale well with long error messages and use the full error message on areas that scale better like the logs output.

For example, currently the time out error easily breaks out of the one sentence limit and also introduces long message that is hard to parse and includes duplicate information (see 01h00m).

🍊 🍊 🍊 🍊

My intention for follow-up PRs is to clean-up the errors the server provides such that we treat them as "publicly facing errors" and include actionable feedback.

Looking forward for this one, @easyCZ! Feel free to loop me in if you need any more feedback on this.

🍋 🍋 🍋 🍋

On the topic of these, the error message should really come directly from the backend.

Also, fully agree. But still masking the type of the error or including a short version of the error message could help to include in small areas like here or in tooltips could be useful.

I think we'll see us move away more from the UI having the error text and more it being sourced directly from the DB as a user facing error.

Sounds good! 🙏

gtsiolis · 2022-04-04T11:43:51Z

Now that this is merged, shall we close any of the related issues linked in the PR description?

Cc @easyCZ @jankeromnes @AlexTugarev

easyCZ · 2022-04-04T12:10:49Z

Thanks for the follow-up @gtsiolis. I believe we can action it as follows:

Some failed prebuilds use a wrong status label #5611 - close, this is indeed resolved by this PR
Some prebuilds use different status on project cards and prebuild page #7384 - already closed
[dashboard, (server?)] Inconsistent derivation of prebuild status #8197 - there remains one more instance of this issue on the Project Settings view. I'm tackling this in [dashboard] Remove Prebuild logs and button from Project Settings #8831 and will close once this lands.

I'll go and close the first now.

gtsiolis · 2022-04-04T12:11:27Z

Thanks, @easyCZ! 🙏

roboquat added do-not-merge/hold do-not-merge/work-in-progress do-not-merge/release-note-label-needed size/XL labels Mar 14, 2022

easyCZ force-pushed the mp/pb-status branch from b8fcaf7 to 351002d Compare March 14, 2022 20:57

roboquat added size/L size/M and removed size/XL size/L labels Mar 14, 2022

easyCZ changed the title ~~WIP: Normalize usage of prebuild status in dashboard~~ [dashboard] Show prebuild status based on prebuild workspace domain record Mar 15, 2022

roboquat added release-note and removed do-not-merge/release-note-label-needed labels Mar 15, 2022

easyCZ changed the title ~~[dashboard] Show prebuild status based on prebuild workspace domain record~~ [dashboard] Show prebuild status based on prebuild workspace record Mar 15, 2022

easyCZ marked this pull request as ready for review March 15, 2022 21:50

easyCZ requested a review from a team March 15, 2022 21:50

roboquat removed the do-not-merge/work-in-progress label Mar 15, 2022

github-actions bot added the team: webapp Issue belongs to the WebApp team label Mar 15, 2022

easyCZ force-pushed the mp/pb-status branch 2 times, most recently from e49a30b to 7ebe21f Compare March 15, 2022 21:52

gtsiolis requested a review from a team March 16, 2022 12:15

github-actions bot added the team: product-design label Mar 16, 2022

Normalize usage of prebuild status in dashboard

9421427

easyCZ force-pushed the mp/pb-status branch from 7ebe21f to 9421427 Compare March 21, 2022 10:04

AlexTugarev reviewed Mar 29, 2022

View reviewed changes

AlexTugarev previously approved these changes Mar 29, 2022

View reviewed changes

jankeromnes reviewed Mar 29, 2022

View reviewed changes

components/dashboard/src/projects/Prebuild.tsx Outdated Show resolved Hide resolved

jankeromnes previously approved these changes Mar 29, 2022

View reviewed changes

Update components/dashboard/src/projects/Prebuild.tsx

7581fb2

Co-authored-by: Jan Keromnes <[email protected]>

easyCZ dismissed stale reviews from jankeromnes and AlexTugarev via 7581fb2 March 29, 2022 09:17

jankeromnes approved these changes Mar 29, 2022

View reviewed changes

roboquat removed the do-not-merge/hold label Mar 29, 2022

roboquat merged commit 3d4e0e4 into main Mar 29, 2022

roboquat deleted the mp/pb-status branch March 29, 2022 09:28

gtsiolis reviewed Mar 29, 2022

View reviewed changes

roboquat added the deployed: webapp Meta team change is running in production label Mar 31, 2022

gtsiolis mentioned this pull request Apr 4, 2022

Some prebuilds use different status on project cards and prebuild page #7384

Closed

easyCZ mentioned this pull request Apr 4, 2022

Some failed prebuilds use a wrong status label #5611

Closed

gtsiolis mentioned this pull request Apr 4, 2022

Improve prebuild status messages including errors on the prebuild page #9111

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dashboard] Show prebuild status based on prebuild workspace record #8805

[dashboard] Show prebuild status based on prebuild workspace record #8805

easyCZ commented Mar 14, 2022 •

edited

Loading

easyCZ commented Mar 16, 2022 •

edited by werft-gitpod-dev-com bot

Loading

easyCZ commented Mar 16, 2022

gtsiolis commented Mar 16, 2022 •

edited by werft-gitpod-dev-com bot

Loading

andrew-farries commented Mar 16, 2022

easyCZ commented Mar 16, 2022

easyCZ commented Mar 21, 2022 •

edited

Loading

AlexTugarev commented Mar 29, 2022 •

edited by werft-gitpod-dev-com bot

Loading

AlexTugarev Mar 29, 2022

AlexTugarev left a comment

jankeromnes left a comment •

edited

Loading

jankeromnes left a comment

easyCZ commented Mar 29, 2022

gtsiolis commented Mar 29, 2022 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis left a comment

gtsiolis Mar 29, 2022

gtsiolis Mar 29, 2022

easyCZ Mar 29, 2022

gtsiolis Mar 29, 2022

gtsiolis Apr 4, 2022

gtsiolis Mar 29, 2022

gtsiolis Mar 29, 2022

gtsiolis Mar 29, 2022

easyCZ commented Mar 29, 2022

gtsiolis commented Mar 29, 2022 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis commented Apr 4, 2022

easyCZ commented Apr 4, 2022

gtsiolis commented Apr 4, 2022 •

edited by werft-gitpod-dev-com bot

Loading

		Prebuild has been cancelled. Either a user cancelled it, or the prebuild rate limit has been
		exceeded. {props.prebuild.error}

[dashboard] Show prebuild status based on prebuild workspace record #8805

[dashboard] Show prebuild status based on prebuild workspace record #8805

Conversation

easyCZ commented Mar 14, 2022 • edited Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

How it looks

Open questions

easyCZ commented Mar 16, 2022 • edited by werft-gitpod-dev-com bot Loading

easyCZ commented Mar 16, 2022

gtsiolis commented Mar 16, 2022 • edited by werft-gitpod-dev-com bot Loading

andrew-farries commented Mar 16, 2022

easyCZ commented Mar 16, 2022

easyCZ commented Mar 21, 2022 • edited Loading

AlexTugarev commented Mar 29, 2022 • edited by werft-gitpod-dev-com bot Loading

Choose a reason for hiding this comment

AlexTugarev left a comment

Choose a reason for hiding this comment

jankeromnes left a comment • edited Loading

Choose a reason for hiding this comment

jankeromnes left a comment

Choose a reason for hiding this comment

easyCZ commented Mar 29, 2022

gtsiolis commented Mar 29, 2022 • edited by werft-gitpod-dev-com bot Loading

gtsiolis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easyCZ commented Mar 29, 2022

gtsiolis commented Mar 29, 2022 • edited by werft-gitpod-dev-com bot Loading

gtsiolis commented Apr 4, 2022

easyCZ commented Apr 4, 2022

gtsiolis commented Apr 4, 2022 • edited by werft-gitpod-dev-com bot Loading

easyCZ commented Mar 14, 2022 •

edited

Loading

easyCZ commented Mar 16, 2022 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis commented Mar 16, 2022 •

edited by werft-gitpod-dev-com bot

Loading

easyCZ commented Mar 21, 2022 •

edited

Loading

AlexTugarev commented Mar 29, 2022 •

edited by werft-gitpod-dev-com bot

Loading

jankeromnes left a comment •

edited

Loading

gtsiolis commented Mar 29, 2022 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis commented Mar 29, 2022 •

edited by werft-gitpod-dev-com bot

Loading

gtsiolis commented Apr 4, 2022 •

edited by werft-gitpod-dev-com bot

Loading