Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Mar 29, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9421427
Normalize usage of prebuild status in dashboard
easyCZ Mar 14, 2022
7581fb2
Update components/dashboard/src/projects/Prebuild.tsx
easyCZ Mar 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions components/dashboard/src/projects/Prebuild.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import PrebuildLogs from "../components/PrebuildLogs";
import Spinner from "../icons/Spinner.svg";
import { getGitpodService, gitpodHostUrl } from "../service/service";
import { TeamsContext, getCurrentTeam } from "../teams/teams-context";
import { PrebuildInstanceStatus } from "./Prebuilds";
import { PrebuildStatus } from "./Prebuilds";
import { shortCommitMessage } from "./render-utils";

export default function () {
Expand Down Expand Up @@ -167,7 +167,7 @@ export default function () {
/>
</div>
<div className="h-20 px-6 bg-gray-50 dark:bg-gray-800 border-t border-gray-200 dark:border-gray-600 flex space-x-2">
{prebuildInstance && <PrebuildInstanceStatus prebuildInstance={prebuildInstance} />}
{prebuildInstance && <PrebuildStatus prebuild={prebuild!} />}
easyCZ marked this conversation as resolved.
Show resolved Hide resolved
<div className="flex-grow" />
{prebuild?.status === "aborted" || prebuild?.status === "timeout" || !!prebuild?.error ? (
<button
Expand Down
56 changes: 55 additions & 1 deletion components/dashboard/src/projects/Prebuilds.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,12 +329,66 @@ export function prebuildStatusIcon(prebuild?: PrebuildWithStatus) {
}
}

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>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case "building":
return <span>Prebuild is currently in progress.</span>;
case "aborted":
return (
<span>
Prebuild has been cancelled. Either a user cancelled it, or the prebuild rate limit has been
exceeded. {props.prebuild.error}
Comment on lines +341 to +342
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</span>
);
case "failed":
return <span>Prebuild failed for system reasons. Please contact support. {props.prebuild.error}</span>;
case "timeout":
return (
<span>
Prebuild timed out. Either the image, or the prebuild tasks took too long. {props.prebuild.error}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</span>
);
case "available":
if (props.prebuild?.error) {
return (
<span>
The tasks executed in the prebuild returned a non-zero exit code. {props.prebuild.error}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Frame 369

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unify first, then improve the messaging.

💯

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

Thanks, @easyCZ! 🏀

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</span>
);
}
return <span>Prebuild completed succesfully.</span>;
default:
return <span>Unknown prebuild status.</span>;
}
}

function formatDuration(milliseconds: number) {
const hours = Math.floor(milliseconds / (1000 * 60 * 60));
return (hours > 0 ? `${hours}:` : "") + moment(milliseconds).format("mm:ss");
}

export function PrebuildInstanceStatus(props: { prebuildInstance?: WorkspaceInstance }) {
export function PrebuildStatus(props: { prebuild: PrebuildWithStatus }) {
const prebuild = props.prebuild;

return (
<div className="flex flex-col space-y-1 justify-center text-sm font-semibold">
<div>
<div className="flex space-x-1 items-center">
{prebuildStatusIcon(prebuild)}
{prebuildStatusLabel(prebuild)}
</div>
</div>
<div className="flex space-x-1 items-center text-gray-400">
<PrebuildStatusDescription prebuild={prebuild} />
</div>
</div>
);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let status = <></>;
let details = <></>;
switch (props.prebuildInstance?.status.phase) {
Expand Down