[dashboard] Adjust Prebuild and Project Configurator pages to spec #5474

jankeromnes · 2021-09-01T08:12:35Z

Aligns the Prebuild & Project Configurator pages with the specs to fix #4423

TODO:

Show detailed Prebuild status and runtime below the logs in the Prebuild and Configurator pages
Refactor server.triggerPrebuild to make branchName optional (picks the default branch and makes e.g. the Prebuilds page load faster) and return information about the triggered Prebuild (so that we can get the logs after triggering in the Configurator)
Refactor the Configurator to use server.triggerPrebuild instead of the "manual" prebuild prefix (thus keeping the prebuild/project association)
Enable Run Prebuild button for Git-based configurations in the Configurator
Fix the Run Prebuild and New Workspace buttons for the Prebuild & Configurator pages, fixes [configuration] Don't suggest/allow starting another prebuild when one is running #5392 and fixes [configurator] Buttons don't do anything #5501

Drive-by fixes:

Rename Detecting project type ... loading screen to Detecting project configuration ... to be more truthful
Add more discoverable 'change' link when adding projects from GitLab/GitHub

/uncc

jankeromnes · 2021-09-01T13:11:38Z

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-jx-fix-prebuild-page.8

jankeromnes · 2021-09-02T16:48:05Z

/approve

…figuration'

…from GitLab/GitHub

…tional for server.triggerPrebuild()

…uildResult

…ver.triggerPrebuild() Also adjust config precedence: - Git-based config > Project DB config > guess config

…pace)

gtsiolis · 2021-09-03T08:38:07Z

Looking at this now! 👀

jankeromnes · 2021-09-03T08:39:11Z

Looking at this now! 👀

Many thanks! 🙏 (Please beware, the PR is currently being re-deployed -- this may cause a brief outage, but shouldn't reset your data or anything you've tested.)

gtsiolis · 2021-09-03T08:47:06Z

Please beware, the PR is currently being re-deployed -- this may cause a brief outage, but shouldn't reset your data or anything you've tested.

That's my favorite way of reviewing a PR. 🙈

AlexTugarev · 2021-09-03T09:11:52Z

Code looks good! 💯

One question to the New Workspace button on the Prebuild page: is it practical? You cannot start a workspace for old prebuilds anyways, so the page is more of a view. Wdyt of removing it from there?

AlexTugarev · 2021-09-03T09:13:34Z

/approve

/hold (because of the question on the New Workspace button. anyways, feel free to unlock)

gtsiolis

One step closer to shipping Teams & Projects! 3️⃣ 2️⃣ 1️⃣ 🚀

Thanks for adding this @jankeromnes! 🏀

gtsiolis · 2021-09-03T08:38:40Z

components/dashboard/src/projects/NewProject.tsx

@@ -385,7 +385,7 @@ export default function NewProject() {

    return (<div className="flex flex-col w-96 mt-24 mx-auto items-center">
        <h1>New Project</h1>
-        <p className="text-gray-500 text-center text-base">Select a git repository on <strong>{provider}</strong>.</p>
+        <p className="text-gray-500 text-center text-base">Select a git repository on <strong>{provider}</strong>. (<a className="gp-link cursor-pointer" onClick={() => setShowGitProviders(true)}>change</a>)</p>


praise: Thanks for making this more visible!

gtsiolis · 2021-09-03T08:45:07Z

components/dashboard/src/projects/NewProject.tsx

@@ -385,7 +385,7 @@ export default function NewProject() {

    return (<div className="flex flex-col w-96 mt-24 mx-auto items-center">
        <h1>New Project</h1>
-        <p className="text-gray-500 text-center text-base">Select a git repository on <strong>{provider}</strong>.</p>
+        <p className="text-gray-500 text-center text-base">Select a git repository on <strong>{provider}</strong>. (<a className="gp-link cursor-pointer" onClick={() => setShowGitProviders(true)}>change</a>)</p>


suggestion: I think using just the verb change could go unoticed and confuse some users. Following the discussion in #5120 (comment) and the second iteration of the designs for repository selection in #4948, we could use a more verbose message here or below the repositories list. What do you think?

Approach A Approach B Second iteration for repository selection

Filed as #5524

gtsiolis · 2021-09-03T08:55:15Z

components/dashboard/src/projects/ConfigureProject.tsx

@@ -166,23 +164,25 @@ export default function () {
        </Suspense>
        {isDetecting && <div className="absolute h-full w-full bg-gray-100 dark:bg-gray-700 flex items-center justify-center space-x-2">
          <img className="h-5 w-5 animate-spin" src={isDark ? SpinnerDark : Spinner} />
-          <span className="font-semibold text-gray-400">Detecting project type ...</span>
+          <span className="font-semibold text-gray-400">Detecting project configuration ...</span>


praise: Thanks for changing this! New message makes more sense for loading an existing project with a git based configuration.

gtsiolis · 2021-09-03T09:01:36Z

components/dashboard/src/projects/ConfigureProject.tsx

@@ -166,23 +164,25 @@ export default function () {
        </Suspense>
        {isDetecting && <div className="absolute h-full w-full bg-gray-100 dark:bg-gray-700 flex items-center justify-center space-x-2">
          <img className="h-5 w-5 animate-spin" src={isDark ? SpinnerDark : Spinner} />
-          <span className="font-semibold text-gray-400">Detecting project type ...</span>
+          <span className="font-semibold text-gray-400">Detecting project configuration ...</span>


question: This is taking noticeably more time sometimes, like 4-5 seconds or more when there's no git based configuration. Expected?

Yes, I think we should look if we can do that ahead of time.

We could also probably optimize the "config inferrer": It currently makes repository requests one by one (e.g. "does a yarn.lock file exist? Oh... does a cargo.toml file exist? Oh... etc.)

Fetching the entire repo structure (not necessarily file content or history) in one go, and using that to respond to the "config inferrer"'s queries would already be a huge performance boost.

Now tracked as #5527

gtsiolis · 2021-09-03T09:11:21Z

components/dashboard/src/projects/Prebuilds.tsx

+                </div>;
+            details = <div className="flex space-x-1 items-center text-gray-400">
+                <img className="h-4 w-4 filter-grayscale" src={StatusRunning} />
+                <span>{!!props.prebuildInstance?.stoppedTime


issue: This is currently showing duration as seconds like 5s but this could look confusing when showing time duration longer than a minute. For example, 306s. Ideally this could surface time duration for 306s as like 05:06 as described in the initial specs in #4948 (comment). However, it should be fine to resolve this in a follow up issue. 🏓

5s 306s

Filed as #5525

gtsiolis · 2021-09-03T09:15:35Z

components/dashboard/src/projects/Prebuilds.tsx

+            </div>;
+    }
+    return <div className="flex flex-col space-y-1 justify-center text-sm font-semibold">
+        <div>{status}</div>


praise: Nice! This reminded me of slots in Vue.

gtsiolis · 2021-09-03T09:21:39Z

components/dashboard/src/projects/Prebuilds.tsx

+            </div>;
+    }
+    return <div className="flex flex-col space-y-1 justify-center text-sm font-semibold">
+        <div>{status}</div>


question: Now that we have proper status updates down here and this component is used as is in the project configurator, wondering if it makes sense to change or remove the status from the header although it could be useful to have the status up there.

If we remove the header instance we'll have to make sure the bottom part of the prebuild output is always visible. Of course, we could leave it as is for now but make sure the designs for both is identical. Alternatively, we could only include the status icon to avoid having duplicated information. What do you think?

BEFORE AFTER (A) AFTER (B)

Excellent point and proposals! Filed as #5526

gtsiolis · 2021-09-03T09:26:06Z

components/dashboard/src/projects/ConfigureProject.tsx

@@ -97,12 +95,17 @@ export default function () {
      const guessedConfigStringPromise = getGitpodService().server.guessProjectConfiguration(project.id);
      const repoConfigString = await getGitpodService().server.fetchProjectRepositoryConfiguration(project.id);
      if (repoConfigString) {
-        // TODO(janx): Link to .gitpod.yml directly instead of just the cloneUrl.
        setIsDetecting(false);
        setEditorMessage(<EditorMessage type="warning" heading="Configuration already exists in git." message="Run a prebuild or open a new workspace to edit project configuration."/>);


issue: This is probably out of the scope of these changes but noticed that it takes a while to redirect to the configuration page once you select a team. Is this expected or needed? Cc @AlexTugarev

True, @AlexTugarev noticed a UI freeze, and I did too today. Maybe it's indeed MonacoEditor? (It's loaded lazily in the Configurator, in order not to make the entire dashboard slower)

gtsiolis · 2021-09-03T09:34:44Z

components/dashboard/src/projects/Prebuild.tsx

    useEffect(() => { document.title = 'Prebuild — Gitpod' }, []);

    return <>
        <Header title={renderTitle()} subtitle={renderSubtitle()} />
        <div className="lg:px-28 px-10 mt-8">
-            <div className="h-96 rounded-xl overflow-hidden bg-gray-100 dark:bg-gray-800 flex flex-col">
-                <PrebuildLogs workspaceId={prebuild?.info?.buildWorkspaceId}/>
+            <div className="rounded-xl overflow-hidden bg-gray-100 dark:bg-gray-800 flex flex-col">


issue: Could not get any prebuild log output here. Expected? 😭

It took some time the first time I opened it. But I, at last, got logs. I think this is unrelated to this PR.

Yes, sometimes logs don't show up at all ("Error: could not find headless logs for 1234"). Sometimes, they don't show up live, but eventually the entire logs show up after the prebuild is complete.

This morning I also had live logs. It's kind of a shame that this isn't more reliable.

We need to look into it (in a separate PR). Can we merge this? 😁

gtsiolis · 2021-09-03T09:41:17Z

components/dashboard/src/projects/Prebuilds.tsx

+            break;
+        case 'stopping': // Fall through
+        case 'stopped':
+            status = <div className="flex space-x-1 items-center text-green-600">


issue: Noticed that the status on the header section does not automatically get updated but you need to reload to the get new status. This could lead to having a discrepancy between these two places where status is surfaced.

Example A Example B

Agreed: The top status is based on the "prebuild" status, while the bottom one is based on the "prebuild workspace instance" status. Normally they should be in sync, but discrepancies can happen during incidents or because of bugs.

roboquat · 2021-09-03T09:44:06Z

LGTM label has been added.

Git tree hash: 79a8555afa90a11325b2bb73d460bdaebf992e7a

svenefftinge · 2021-09-03T11:20:12Z

One question to the New Workspace button on the Prebuild page: is it practical? You cannot start a workspace for old prebuilds anyways, so the page is more of a view. Wdyt of removing it from there?

I'd opt to leave it here. I was just using it and didn't have any doubts that it was handy there. It should maybe show what it does (Open workspace on branch branch-name) in the tooltip.

svenefftinge

Such an improvement! 💯 I can feel how all this could be useful 😁

The small nits should be done in small separate PRs. Based on all issues that we assign ourselves before taking them.

roboquat · 2021-09-03T11:26:22Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: #4423

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:

~~components/dashboard/OWNERS~~ [AlexTugarev,jankeromnes,svenefftinge]
~~components/gitpod-protocol/OWNERS~~ [AlexTugarev,jankeromnes,svenefftinge]
~~components/server/OWNERS~~ [AlexTugarev,jankeromnes,svenefftinge]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jankeromnes · 2021-09-03T11:42:41Z

Many thanks for the awesome feedback! ❤️

I agree with merging this now, and filing/doing the improvements as follow-ups.

One question to the New Workspace button on the Prebuild page: is it practical? You cannot start a workspace for old prebuilds anyways, so the page is more of a view. Wdyt of removing it from there?

Good point. Like @svenefftinge, I'd prefer keeping it for now, but independently of whether we want to keep it, I believe the link is actually incorrect 😬 (it points to the context URL of the prebuild, which becomes incorrect when you push new commits on top of the branch -- instead, it should probably link to the commit URL instead).

/unhold

svenefftinge · 2021-09-03T11:43:57Z

instead, it should probably link to the commit URL instead

It should link to the branch but this needs to be communicated somehow.
But I see the point now. Having a button on an old outdated prebuild would be confusing.

jankeromnes · 2021-09-03T11:45:56Z

But I see the point now. Having a button on an old outdated prebuild would be confusing.

Or, we can still link to the exact (potentially outdated) prebuild by using the commit context URL.

But maybe after using it for a bit we'll know more about what's better here.

EDIT: Let's move this discussion to this follow-up issue: #5523

roboquat added do-not-merge/work-in-progress size/L labels Sep 1, 2021

jankeromnes force-pushed the jx/fix-prebuild-page branch 2 times, most recently from 0bcb74f to 15ec7bf Compare September 1, 2021 10:34

jankeromnes changed the title ~~[dashboard] Adjust Prebuild page to specs~~ [dashboard] Adjust Prebuild and Project Configurator pages to spec Sep 1, 2021

jankeromnes force-pushed the jx/fix-prebuild-page branch from f2f16d6 to f68e1ed Compare September 1, 2021 13:27

gtsiolis added the feature: teams and projects [DEPRECATED] Please, use feature: organizations or feature: projects labels instead. label Sep 1, 2021

jankeromnes force-pushed the jx/fix-prebuild-page branch 3 times, most recently from 191f19f to 210fd75 Compare September 1, 2021 16:19

gtsiolis added 🧑‍🚀 crew: teams and projects and removed feature: teams and projects [DEPRECATED] Please, use feature: organizations or feature: projects labels instead. labels Sep 1, 2021

jankeromnes force-pushed the jx/fix-prebuild-page branch 2 times, most recently from b629234 to eaeb191 Compare September 2, 2021 06:41

jankeromnes mentioned this pull request Sep 2, 2021

[configurator] Buttons don't do anything #5501

Closed

jankeromnes force-pushed the jx/fix-prebuild-page branch from eaeb191 to bf7945e Compare September 2, 2021 12:52

roboquat added size/XL and removed size/L labels Sep 2, 2021

jankeromnes force-pushed the jx/fix-prebuild-page branch from bf7945e to 318b863 Compare September 2, 2021 16:00

jankeromnes mentioned this pull request Sep 2, 2021

[configuration] Don't suggest/allow starting another prebuild when one is running #5392

Closed

jankeromnes force-pushed the jx/fix-prebuild-page branch from 318b863 to 8620c38 Compare September 2, 2021 16:38

roboquat added the approved label Sep 2, 2021

jankeromnes force-pushed the jx/fix-prebuild-page branch from 8620c38 to 1ffda4b Compare September 2, 2021 16:53

jankeromnes added 5 commits September 3, 2021 07:43

[dashboard] Rename 'Detecting project type' to 'Detecting project con…

bdb5278

…figuration'

[dashboard] Add more discoverable 'change' link when adding projects …

2781462

…from GitLab/GitHub

[dashboard][server] Accelerate Prebuilds page by making branchName op…

302e763

…tional for server.triggerPrebuild()

[server][protocol] Make server.triggerPrebuild() return the StartPreb…

3b896ac

…uildResult

[dashboard] Enable 'Run Prebuild' button for Git-based configurations

ff1a0bf

jankeromnes added 3 commits September 3, 2021 08:32

[dashboard] Refactor Project Configurator from 'manual' prefix to ser…

788e766

…ver.triggerPrebuild() Also adjust config precedence: - Git-based config > Project DB config > guess config

[dashboard] Adjust Prebuild page to specs

1f4f75b

[server][dashboard] Fix PrebuildLogs buttons (Run Prebuild, New Works…

12afee9

…pace)

jankeromnes force-pushed the jx/fix-prebuild-page branch from c6be8a8 to 12afee9 Compare September 3, 2021 08:33

roboquat added the do-not-merge/hold label Sep 3, 2021

gtsiolis approved these changes Sep 3, 2021

View reviewed changes

roboquat assigned gtsiolis Sep 3, 2021

roboquat added the lgtm label Sep 3, 2021

svenefftinge approved these changes Sep 3, 2021

View reviewed changes

roboquat assigned svenefftinge Sep 3, 2021

roboquat removed the do-not-merge/hold label Sep 3, 2021

roboquat merged commit d418735 into main Sep 3, 2021

roboquat deleted the jx/fix-prebuild-page branch September 3, 2021 11:43

svenefftinge mentioned this pull request Sep 3, 2021

Improve Inferrer Performance #5527

Closed

gtsiolis mentioned this pull request Sep 8, 2021

Allow users to cancel pending or running prebuilds for projects #5612

Closed

[dashboard] Adjust Prebuild and Project Configurator pages to spec #5474

[dashboard] Adjust Prebuild and Project Configurator pages to spec #5474

Conversation

jankeromnes commented Sep 1, 2021 • edited

jankeromnes commented Sep 1, 2021 • edited by werft-gitpod-dev-com bot

jankeromnes commented Sep 2, 2021

gtsiolis commented Sep 3, 2021 • edited by werft-gitpod-dev-com bot

jankeromnes commented Sep 3, 2021 • edited by werft-gitpod-dev-com bot

gtsiolis commented Sep 3, 2021 • edited by werft-gitpod-dev-com bot

AlexTugarev commented Sep 3, 2021 • edited by werft-gitpod-dev-com bot

AlexTugarev commented Sep 3, 2021

gtsiolis left a comment • edited

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

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

jankeromnes Sep 3, 2021 • edited

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

roboquat commented Sep 3, 2021

svenefftinge commented Sep 3, 2021

svenefftinge left a comment

Choose a reason for hiding this comment

roboquat commented Sep 3, 2021

jankeromnes commented Sep 3, 2021 • edited by werft-gitpod-dev-com bot

svenefftinge commented Sep 3, 2021 • edited

jankeromnes commented Sep 3, 2021 • edited

jankeromnes commented Sep 1, 2021 •

edited

jankeromnes commented Sep 1, 2021 •

edited by werft-gitpod-dev-com bot

gtsiolis commented Sep 3, 2021 •

edited by werft-gitpod-dev-com bot

jankeromnes commented Sep 3, 2021 •

edited by werft-gitpod-dev-com bot

gtsiolis commented Sep 3, 2021 •

edited by werft-gitpod-dev-com bot

AlexTugarev commented Sep 3, 2021 •

edited by werft-gitpod-dev-com bot

gtsiolis left a comment •

edited

jankeromnes Sep 3, 2021 •

edited

jankeromnes commented Sep 3, 2021 •

edited by werft-gitpod-dev-com bot

svenefftinge commented Sep 3, 2021 •

edited

jankeromnes commented Sep 3, 2021 •

edited