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.

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

Prebuilds Page #4876

Merged
merged 3 commits into from
Jul 26, 2021
Merged

Prebuilds Page #4876

merged 3 commits into from
Jul 26, 2021

Conversation

AlexTugarev
Copy link
Member

@AlexTugarev AlexTugarev commented Jul 20, 2021

Project Overview

Showing branch details.

Screen Shot 2021-07-22 at 12 34 02


Prebuilds

Listing all prebuilds of a project.

This is the first iteration on #4515

Screen Shot 2021-07-22 at 12 34 07


Prebuild

To show all details on a prebuild. Currently just including the existing <PrebuildLogs> components. The plan is to restyle it afterwards.

Screen Shot 2021-07-23 at 14 09 03

Known Issue

We've no build numbers (yet)

@AlexTugarev AlexTugarev force-pushed the at/prebuild-page branch 4 times, most recently from ab1474d to cccc3ee Compare July 23, 2021 07:14
jankeromnes added a commit to jankeromnes/gitpod-staging-prebuilds that referenced this pull request Jul 23, 2021
@jankeromnes
Copy link
Contributor

jankeromnes commented Jul 23, 2021

Wow, very cool! 🙌 I was able to see a newly triggered prebuild, and then check on its logs. I love it 😍

Screenshot 2021-07-23 at 14 39 23 Screenshot 2021-07-23 at 14 39 35

Thoughts:

  • It took quite a long time before my prebuild showed up (it did well after finishing). Do you know why there is a long delay between the trigger of a prebuild and it showing up? (Is that db-sync?)
  • When adding a new project, I'd love to see all the older prebuilds as well. Can't we also surface older prebuilds? (E.g. based on the clone URL).

Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

This is amazing! 😁 Thanks again for powering through this and displaying prebuilds & their status/logs! 🙏

This is quite a big PR. I've done a quick pass on the code, and left some comments/suggestions/thoughts in-line.

Also, have a fantastic weekend! ☀️

components/dashboard/src/App.tsx Show resolved Hide resolved
components/dashboard/src/components/PrebuildLogs.tsx Outdated Show resolved Hide resolved
Comment on lines +1 to +3
<svg width="8" height="14" viewBox="0 0 8 14" fill="none" xmlns="http://www.w3.org/2000/svg">
<path fill-rule="evenodd" clip-rule="evenodd" d="M4 0C4.26522 5.96046e-08 4.51957 0.105357 4.70711 0.292893L7.70711 3.29289C8.09763 3.68342 8.09763 4.31658 7.70711 4.70711C7.31658 5.09763 6.68342 5.09763 6.29289 4.70711L4 2.41421L1.70711 4.70711C1.31658 5.09763 0.683417 5.09763 0.292893 4.70711C-0.0976311 4.31658 -0.097631 3.68342 0.292893 3.29289L3.29289 0.292893C3.48043 0.105357 3.73478 0 4 0ZM0.292893 9.29289C0.683417 8.90237 1.31658 8.90237 1.70711 9.29289L4 11.5858L6.29289 9.29289C6.68342 8.90237 7.31658 8.90237 7.70711 9.29289C8.09763 9.68342 8.09763 10.3166 7.70711 10.7071L4.70711 13.7071C4.31658 14.0976 3.68342 14.0976 3.29289 13.7071L0.292893 10.7071C-0.0976311 10.3166 -0.0976311 9.68342 0.292893 9.29289Z" fill="#57534E"/>
</svg>
Copy link
Contributor

Choose a reason for hiding this comment

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

Request: Please always optimize new SVG files via https://jakearchibald.github.io/svgomg/ (hint: you can paste the SVG text, and then copy the optimized SVG text, without having to mess with files/uploads/downloads)

components/dashboard/src/projects/Prebuild.tsx Outdated Show resolved Hide resolved
components/gitpod-db/src/project-db.ts Outdated Show resolved Hide resolved
}

public validate(): { msg?: string, valid: boolean } {
const v = validate(this.instanceID);
if (v.valid) {
// const v = validate(this.instanceID);
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder: Please revert this commit before merging.

components/server/src/auth/resource-access.ts Outdated Show resolved Hide resolved
components/gitpod-protocol/src/gitpod-service.ts Outdated Show resolved Hide resolved
components/gitpod-protocol/src/teams-projects-protocol.ts Outdated Show resolved Hide resolved
@AlexTugarev AlexTugarev changed the title WIP Prebuilds Page Prebuilds Page Jul 26, 2021
@AlexTugarev
Copy link
Member Author

@jankeromnes

It took quite a long time before my prebuild showed up (it did well after finishing). Do you know why there is a long delay between the trigger of a prebuild and it showing up? (Is that db-sync?)

Event handling for new prebuilds is not part of this PR. You'd need to reload the page manually to see changes.

When adding a new project, I'd love to see all the older prebuilds as well. Can't we also surface older prebuilds? (E.g. based on the clone URL).

Though technically possible, I'd argue to not pull in this complexity. Having a clear relation between a prebuilt workspace and a project makes the implementation cleaner then dealing with branch mismatches and the like. OTOH we can revisit this when migrating existing app installations, wdyt?

@AlexTugarev AlexTugarev marked this pull request as ready for review July 26, 2021 08:57
@gtsiolis
Copy link
Contributor

gtsiolis commented Jul 26, 2021

/werft run

👍 started the job as gitpod-build-at-prebuild-page.57

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Great work @AlexTugarev! 🌟 🌟 🌟

Feels so good to see prebuilds on the dashboard! 🧡

Added a few comments below, let me know if anything sounds confusing and feel free to open follow up issues to address smaller issues.

@@ -36,6 +36,7 @@ const ConfigureProject = React.lazy(() => import(/* webpackPrefetch: true */ './
const Projects = React.lazy(() => import(/* webpackPrefetch: true */ './projects/Projects'));
const Project = React.lazy(() => import(/* webpackPrefetch: true */ './projects/Project'));
const Prebuilds = React.lazy(() => import(/* webpackPrefetch: true */ './projects/Prebuilds'));
const Prebuild = React.lazy(() => import(/* webpackPrefetch: true */ './projects/Prebuild'));
const Settings = React.lazy(() => import(/* webpackPrefetch: true */ './projects/Settings'));
Copy link
Contributor

Choose a reason for hiding this comment

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

question: We no longer need the Settings tab for this iteration, right? If so, let's remove this here or in a follow up issue.

<div>
<div className="text-base text-gray-900 dark:text-gray-50 font-medium">{moment(p.startedAt).fromNow()}</div>
<p>{p.startedBy}</p>
<div className="text-base text-gray-500 dark:text-gray-50 font-medium mb-1">{shortCommitMessage(p.changeTitle)}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: We'll need to truncate this element to avoid breaking the layout like the following:

Screenshot 2021-07-26 at 3 28 09 PM

<div className="flex justify-end">
<div className="h-full my-auto flex self-center opacity-0 group-hover:opacity-100">
{!r.inUse ? (
<button className="primary" onClick={() => setSelectedRepo(r.name)}>Select</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: Specs for repository selection are being updated and we no longer need the primary button on hover, see #4948. However, this is probably out of the scope these changes. For the secondary option below we can use a tooltip on hover to indicate a projects is already taken. 💡

<ItemField className="flex items-center">
<div>
<div className="text-base text-gray-900 dark:text-gray-50 font-medium mb-1">
{branchName}
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Do you think adding a label for the default branch here as seen in the initial specs would help? Currently the entry mysteriously stays on top with no indication to the user on why this happens.

One approach could be to add a label next to the default branch. Alternatively, we could all sort branches, including the default branch, with updated timestamp. Additionally, we could consider adding back the New Workspace primary button on the top of this list near list fitlers. 💭

<div className="text-base text-gray-900 dark:text-gray-50 font-medium mb-1">
{branchName}
</div>
<p>Updated _ minutes ago</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Do you think this piece of information is possible to retrieve for GitHub, GitLab, and Bitbucket?

</ItemField>
<ItemField className="flex items-center">
<div className="text-base text-gray-900 dark:text-gray-50 font-medium uppercase mb-1 cursor-pointer" onClick={() => prebuild && openPrebuild(prebuild)}>
{prebuild ? (<><div className="inline-block align-text-bottom mr-2 w-4 h-4">{statusIcon}</div>{status}</>) : (<span>–</span>)}
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Skipping the hyphen here sounds also ok.

</div>
<p className="mx-2 my-auto">·</p>
<div className="my-auto">
<p>{startedByAvatar}Triggered {moment(prebuild.startedAt).fromNow()}</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Is it expected to see the avatar missing from everywhere?

if (!prebuild) {
return "unknown prebuild";
}
return (<h1 className="tracking-tight">{prebuild.branch} <span className="text-gray-200">#{prebuild.branchPrebuildNumber}</span></h1>);
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Minor color fix. 🖍️

Suggested change
return (<h1 className="tracking-tight">{prebuild.branch} <span className="text-gray-200">#{prebuild.branchPrebuildNumber}</span></h1>);
return (<h1 className="tracking-tight">{prebuild.branch} <span className="text-gray-400">#{prebuild.branchPrebuildNumber}</span></h1>);

title: string;
subtitle: string;
title: string | React.ReactElement;
subtitle: string | React.ReactElement;
}

export default function Header(p: HeaderProps) {
return <div className="lg:px-28 px-10 border-gray-200 dark:border-gray-800">
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: We'll need a spacing of 4 below the header here. mayeb this is something we need to add on the header component?

BEFORE AFTER
header-before header-after

@@ -36,6 +36,7 @@ const ConfigureProject = React.lazy(() => import(/* webpackPrefetch: true */ './
const Projects = React.lazy(() => import(/* webpackPrefetch: true */ './projects/Projects'));
const Project = React.lazy(() => import(/* webpackPrefetch: true */ './projects/Project'));
const Prebuilds = React.lazy(() => import(/* webpackPrefetch: true */ './projects/Prebuilds'));
const Prebuild = React.lazy(() => import(/* webpackPrefetch: true */ './projects/Prebuild'));
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: Fetching active branches and prebuilds requires a noticable amount of time, 5 seconds or more. Is there anything we could do in this iteration to make this smoother? If not, let's add a follow up issue for adding a loading indicator in a future iteration. ⏱️

Copy link
Member

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

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

LGTM. I think we can merge this and continue work in smaller steps

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: c38192ab1a8f78d1522a3c24f99166225951bb1f

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlexTugarev, svenefftinge

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dashboard] Project/Prebuilds Page [Dashboard] Project Page (Overview)
5 participants