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

Jump to bottom

Allow enabling/disabling Incremental Prebuilds in Project Settings #7031

Merged
merged 5 commits into from
Dec 14, 2021
Merged

Allow enabling/disabling Incremental Prebuilds in Project Settings #7031

merged 5 commits into from
Dec 14, 2021

Conversation

jankeromnes
Copy link
Contributor

@jankeromnes jankeromnes commented Dec 2, 2021 •
edited
Loading

Description

Allow enabling/disabling Incremental Prebuilds in Project Settings:

  • Add a new Project Settings menu containing:
    • The existing Configuration page
    • Another page with a "Enable Incremental Prebuilds" checkbox
  • If it's enabled for a Project, make new Prebuilds incremental
  • In the Prebuild page, show when a Prebuild is incremental
  • Drive-by: Improve formatting of Prebuild duration
  • Drive-by: Correctly show 'timed out' status in Prebuild Logs

Related Issue(s)

Fixes #6727

How to test

  1. Add a Project
  2. Start a Prebuild, and wait for it to be successful
  3. Only after it finished successfully, push a new commit to the same branch
  4. This should trigger a new Prebuild, but the new Prebuild should not be incremental
  5. Next, in the Project's Settings, enable "Use Incremental Prebuilds"
  6. Push another commit to the same branch
  7. The newly triggered Prebuild should be incremental

Release Notes

Allow enabling/disabling Incremental Prebuilds in Project Settings
Also improve Prebuild Logs UX (show incremental status, format duration nicely, correctly show 'timed out' status)

Documentation

/uncc

@roboquat roboquat added release-note team: webapp Issue belongs to the WebApp team size/L labels Dec 2, 2021
@jankeromnes
Copy link
Contributor Author

Work in progress:

New Project Settings page
Screenshot 2021-12-02 at 14 51 57
Non-incremental Prebuild Incremental Prebuild
Screenshot 2021-12-02 at 14 52 38 Screenshot 2021-12-02 at 14 52 58

@jankeromnes jankeromnes requested a review from gtsiolis December 2, 2021 13:57
@jankeromnes
Copy link
Contributor Author

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

@gtsiolis Please take a look at the UX when you have time 🙏

@geropl Please take a look at the code change when you have time 🙏

@jankeromnes jankeromnes requested a review from geropl December 3, 2021 08:33
@geropl
Copy link
Member

geropl commented Dec 3, 2021

Starting review now...

@geropl
Copy link
Member

geropl commented Dec 3, 2021

First impressions: This is set-up for confusion (Settings vs Configuration):
image
I had to look three times to understand where I am and to grok what the difference is.

@geropl
Copy link
Member

geropl commented Dec 3, 2021 •
edited
Loading

@jankeromnes Whoops. What did I break? 😬 No prebuild ran yet.
I faintly recall we had two places in the code that check for these tab-names. Maybe I see the fall-through for an unhandled case? 🤔
image

@geropl
Copy link
Member

geropl commented Dec 3, 2021 •
edited
Loading

@jankeromnes Finished 1st round! Nice to see this landing. 🛬

I have no hard feelings about the UX "settings vs configuration". But if 1/2 hour of discussion might solve the (potential) confusion now (maybe with @gtsiolis ?) I feel that'd be time well invested (so we don't have to come back to this later).

@jankeromnes
Copy link
Contributor Author

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

@jankeromnes Finished 1st round! Nice to see this landing. 🛬

Woohoo! Many thanks for the excellent review @geropl 🙏

I have no hard feelings about the UX "settings vs configuration". But if 1/2 hour of discussion might solve the (potential) confusion now (maybe with @gtsiolis ?) I feel that'd be time well invested (so we don't have to come back to this later).

Good point that we should probably resolve this within this Pull Request. I was sort of assuming that the Configuration page might be completely removed soon (e.g. superseded by #6921), or that we could at least move it into Settings (e.g. maybe Project Settings can have multiple sub-pages like the User Settings and Team Settings, that seems like common practice). Happy to chat about this, or to implement whatever you think is best @gtsiolis 😁

@geropl
Copy link
Member

geropl commented Dec 3, 2021

I was sort of assuming that the Configuration page might be completely removed soon (e.g. superseded by #6921), or that we could at least move it into Settings

Ah, excellent points! 💡

@gtsiolis
Copy link
Contributor

gtsiolis commented Dec 7, 2021 •
edited by werft-gitpod-dev-com bot
Loading

Looking at this now! 👀

gtsiolis
gtsiolis approved these changes Dec 7, 2021
View reviewed changes
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.

Thanks, @jankeromnes!

Natural next steps could be to introduce an alert component that nudges users to enable or disable incremental prebuilds based on average prebuild duration maybe of the last few prebuilds. 💭

Approving to unblock merging but holding in case we could resolve the review comments below. Let me know if I missed anything worth discussing here or in sync.

/hold

components/dashboard/src/Menu.tsx Outdated Show resolved Hide resolved
components/dashboard/src/Menu.tsx Outdated Show resolved Hide resolved
components/dashboard/src/projects/Prebuilds.tsx Show resolved Hide resolved
components/dashboard/src/projects/ProjectSettings.tsx Outdated Show resolved Hide resolved
components/dashboard/src/projects/ProjectSettings.tsx Outdated Show resolved Hide resolved
components/dashboard/src/projects/ProjectSettings.tsx Outdated Show resolved Hide resolved
components/dashboard/src/projects/ProjectSettings.tsx Outdated
<div className="app-container">
<div className="mt-8">
<h3>Incremental Prebuilds</h3>
<p className="text-gray-500 pb-4">When possible, use an earlier successful prebuild as a base to create new prebuilds. This can make your prebuilds significantly faster, especially if they normally take longer than 10 minutes. <a className="gp-link" href="https://www.gitpod.io/changelog/faster-incremental-prebuilds">Learn more</a></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Thanks @nisarhassan12 for adding the individual changelog entry pages[1][2][3]!

@roboquat roboquat added the lgtm label Dec 7, 2021
@roboquat
Copy link
Contributor

roboquat commented Dec 7, 2021

LGTM label has been added.

Git tree hash: c5ad5c121ae46a9e61b4e3e13eb219fe2bb4d75d

@geropl
Copy link
Member

geropl commented Dec 10, 2021

/lgtm

@jankeromnes I keep the hold in case you want to tackle #7031 (comment) within this PR.

@jankeromnes jankeromnes force-pushed the jx/incremental-prebuild-settings branch from 440b723 to 7212bb4 Compare December 13, 2021 15:26
@roboquat roboquat removed the lgtm label Dec 13, 2021
@jankeromnes jankeromnes force-pushed the jx/incremental-prebuild-settings branch from 7212bb4 to dd4d2b8 Compare December 13, 2021 15:49
@jankeromnes jankeromnes force-pushed the jx/incremental-prebuild-settings branch from dd4d2b8 to c8c3da4 Compare December 13, 2021 17:19
@jankeromnes
Copy link
Contributor Author

/unhold

@geropl
Copy link
Member

geropl commented Dec 14, 2021

/lgtm

Code is fine, and tested last time! 👍

@roboquat roboquat added the lgtm label Dec 14, 2021
@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: ec9da6fb245e4f3595a832f5ac2fc39bfce1d6ec

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geropl, gtsiolis

Associated issue: #6727

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

@roboquat roboquat merged commit 704c467 into main Dec 14, 2021
@roboquat roboquat deleted the jx/incremental-prebuild-settings branch December 14, 2021 07:42
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Dec 14, 2021
This was referenced Dec 14, 2021
Closed
Closed
Merged
@gtsiolis gtsiolis mentioned this pull request Jan 3, 2022
Closed
@gtsiolis gtsiolis mentioned this pull request Jan 18, 2022
Merged
This was referenced Jan 25, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geropl geropl geropl left review comments

@gtsiolis gtsiolis gtsiolis approved these changes

Assignees

@gtsiolis gtsiolis

@geropl geropl

Labels
approved deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note size/XL team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow enabling/disabling Incremental Prebuilds in a Project's settings
4 participants
@jankeromnes @geropl @gtsiolis @roboquat