Add `prebuildDefaultBranchOnly` setting – EXP-574 #18716

AlexTugarev · 2023-09-14T11:16:28Z

Description

This adds a new setting to allow limit prebuilds to run on the default branch only. It will be enabled by default for new projects when prebuilds get enabled. For existing projects this won't have any limiting effect.

Default branch	All branches

Commits on different branches	Prebuilds the default branch only

Test thoroughly with a couple of SCM integrations

Summary generated by Copilot

`🤖 Generated by Copilot at 2645ab6`

This pull request adds a new setting to control whether prebuilds are triggered only for the default branch of a project. This setting is part of the experimental public API and can be configured per project. The setting is added to the protobuf definition, the Go and TypeScript types, the API response, and the project creation logic. The default value is true.

Related Issue(s)

Fixes EXP-574

How to test

see unit tests in prebuild-manager.spec.ts
create a project for a test repository of yours and enable prebuilds in settings
explore the behavior on new commits using https://at-defaultb846df8cc3.preview.gitpod-dev.com/projects/<YOURPROJECT>/events, e.g.

Documentation

Preview status

Gitpod was successfully deployed to your preview environment.

🏷️ Name - at-defaultb846df8cc3
🔗 URL - at-defaultb846df8cc3.preview.gitpod-dev.com/workspaces.
📚 Documentation - See our internal documentation for information on how to interact with your preview environment.
📦 Version - at-default-branch-only-gha.16927
🗒️ Logs - GCP Logs Explorer

Build Options

Build

/werft with-werft
Run the build with werft instead of GHA
leeway-no-cache
/werft no-test
Run Leeway with --dont-test

Publish

/werft publish-to-npm
/werft publish-to-jb-marketplace

Installer

analytics=segment
with-dedicated-emulation
workspace-feature-flags
Add desired feature flags to the end of the line above, space separated

Preview Environment / Integration Tests

/werft with-local-preview
If enabled this will build install/preview
/werft with-preview
/werft with-large-vm
/werft with-gce-vm
If enabled this will create the environment on GCE infra
with-integration-tests=all
Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh. If enabled, with-preview and with-large-vm will be enabled.
with-monitoring

/hold

gtsiolis

Woohoo! Thanks, @AlexTugarev. 🌮

Left a first round of comments about the design changes here. Feedback is welcome!

components/dashboard/src/projects/ProjectSettings.tsx

AlexTugarev · 2023-09-18T10:14:07Z

components/gitpod-protocol/src/teams-projects-protocol.ts

@@ -74,6 +82,20 @@ export namespace Project {
        return project.settings.enablePrebuilds;
    }

+    export function getPrebuildBranchStrategy(project: Project): ProjectSettings.PrebuildBranchStrategy {
+        if (typeof project.settings?.enablePrebuilds === "undefined") {
+            return "defaultBranch"; // default value for `settings.prebuildDefaultBranchOnly`


TODO: clarify if the change of the default behavior for pre-existing projects is acceptable. This would disable prebuilds on branches other than the default one once deployed. We might need a nudge in-app, so this change is not causing trouble.

We can return allBranches here to mimic the current behavior for pre-existing projects and postpone this decision/change to a later point in time.

I'm going to change this as mentioned before, i.e. if prebuild settings aren't persisted use allBranches as default strategy.

selfcontained

UI code looks pretty good to me. I think we could improve it by adjusting how we handle updating project settings so it's using a mutation and updating optimistically.

selfcontained · 2023-09-19T13:56:39Z

components/dashboard/src/projects/ProjectSettings.tsx

+                        >
+                            <option value="defaultBranch">Default branch (e.g. main)</option>
+                            <option value="allBranches">All branches</option>
+                            {/* <option value="selectedBranches">Matched by pattern</option> */}


can this be removed?

components/dashboard/src/projects/ProjectSettings.tsx

svenefftinge · 2023-09-21T07:12:31Z

components/gitpod-protocol/src/teams-projects-protocol.ts

+     * Wether prebuilds (if enabled) should only be started on the default branch.
+     * Defaults to `true` on project creation.
+     */
+    prebuildDefaultBranchOnly?: boolean;


here (similar to enablePrebuilds above) we introduce an optional property where undefined means true. I think that is confusing. Can we make it prebuildAllBranches?: boolean or as we are going to allow a pattern already use a pattern internally and make it *?

Alternatively, introduce a non-boolean property (i.e. the strategy thing below).

I would like to remove the optional from prebuildAllBranches. This was a concern of rolling out to Gitpod Cloud.

Once we agree on a data migration path, we can remove the ?.

I'm also OK with using constant to represent those states, e.g. all-branches, default-branch, matched-branches. Actually this would be even simpler, as we are using a <select> in frontend, which maps to a tuple of boolean to represents this state.

svenefftinge · 2023-09-21T07:27:16Z

components/server/src/prebuilds/prebuild-manager.spec.ts

@@ -0,0 +1,199 @@
+/**


Thanks for writing a test ❤️

svenefftinge · 2023-09-21T07:30:08Z

components/server/src/prebuilds/github-enterprise-app.ts

-            ) {
+            const prebuildPrecondition = this.prebuildManager.checkPrebuildPrecondition({ config, project, context });
+
+            const shouldRun = Project.hasPrebuildSettings(project)


why is this check not part of checkPrebuildPrecondition?

I see you use this to check if we need to call appRules. We could move them to checkPrebuildCondition as well as they seem to also only use the passed-in state (config).
OTOH I understood we would simple deprecate and remove the appRules config. Why don'T we do that already?

As we are to remove appRules, which are also marked as deprecated here, I wanted to make it easier to remove those parts. This means, the new check has to be complete if appRules are removed. But this also means, for pre-existing projects appRules should continue working. Once the data migration is done, this code paths are no longer used and we can remove it easily.

svenefftinge · 2023-09-21T07:33:59Z

components/server/src/prebuilds/github-app-rules.ts

+    /**
+     *
+     * @deprecated
+     */


Would it make sense to move this logic to checkPrebuildPrecondition?

basically same as #18716 (comment)

AlexTugarev · 2023-09-22T09:45:27Z

W.r.t. #18716 (comment)

We're going to create a follow-up with an aligned shape and structure of prebuild settings which would also include a lazy data migration for existing projects. There is agreement to not hold up this change, as we the internally discussed changes would be applicable in the follow-up as well.

AlexTugarev · 2023-09-22T09:58:38Z

/unhold

roboquat added do-not-merge/work-in-progress do-not-merge/hold size/XXL labels Sep 14, 2023

AlexTugarev changed the base branch from main to at/enablePrebuilds-settings September 14, 2023 11:16

roboquat added size/XS and removed size/XXL labels Sep 14, 2023

AlexTugarev changed the title ~~At/default-branch-only~~ Add prebuildDefaultBranchOnly setting – EXP-574 Sep 14, 2023

AlexTugarev force-pushed the at/default-branch-only branch from 2645ab6 to e5c8e3a Compare September 14, 2023 11:18

roboquat added size/M and removed size/XS labels Sep 14, 2023

AlexTugarev force-pushed the at/default-branch-only branch from cb6c888 to f08fbf5 Compare September 14, 2023 13:50

AlexTugarev marked this pull request as ready for review September 14, 2023 13:53

AlexTugarev requested a review from a team as a code owner September 14, 2023 13:53

roboquat removed the do-not-merge/work-in-progress label Sep 14, 2023

github-actions bot added the team: team-experience label Sep 14, 2023

gtsiolis reviewed Sep 14, 2023

View reviewed changes

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

components/dashboard/src/projects/ProjectSettings.tsx Show resolved Hide resolved

Base automatically changed from at/enablePrebuilds-settings to main September 15, 2023 07:07

roboquat added size/XXL and removed size/M labels Sep 15, 2023

AlexTugarev force-pushed the at/default-branch-only branch 2 times, most recently from 01f1ec4 to ad8298e Compare September 15, 2023 08:06

roboquat added size/M and removed size/XXL labels Sep 15, 2023

AlexTugarev marked this pull request as draft September 15, 2023 09:23

roboquat added the do-not-merge/work-in-progress label Sep 15, 2023

AlexTugarev force-pushed the at/default-branch-only branch from ad8298e to b9df6aa Compare September 15, 2023 11:48

AlexTugarev marked this pull request as ready for review September 18, 2023 09:49

roboquat removed the do-not-merge/work-in-progress label Sep 18, 2023

AlexTugarev commented Sep 18, 2023

View reviewed changes

roboquat removed the size/M label Sep 18, 2023

AlexTugarev force-pushed the at/default-branch-only branch from 85219af to cd5e720 Compare September 19, 2023 07:35

gtsiolis requested a review from a team September 19, 2023 09:27

selfcontained reviewed Sep 19, 2023

View reviewed changes

AlexTugarev added 9 commits September 20, 2023 08:57

add prebuildDefaultBranchOnly to Project.settings

8012bc7

evaluate prebuildDefaultBranchOnly in PrebuildManager

1f34982

add "Prebuild the default branch only" checkbox

5ce8104

fix missin PAPI mapping 🤯

af8e407

fix PrebuildDefaultBranchOnly in gitpod-service.go

f17e190

turn text input into select input

4c814ee

update styles on project settings page

9a8e9df

mimic old behavior for pre-existing projects

623b5a5

use getPrebuildBranchStrategy in PrebuildManager

d53d9eb

AlexTugarev force-pushed the at/default-branch-only branch from cd5e720 to d53d9eb Compare September 20, 2023 08:59

add tests for PrebuildManager.prebuildPrecondition

9aac05a

roboquat added size/XL and removed size/L labels Sep 20, 2023

AlexTugarev added 2 commits September 20, 2023 11:15

fix getPrebuildEvents

fc3e77d

explicit settings under the project should have precedence

6ec7dae

svenefftinge reviewed Sep 21, 2023

View reviewed changes

svenefftinge approved these changes Sep 21, 2023

View reviewed changes

render changes to project settings immediately

31a1409

roboquat removed the do-not-merge/hold label Sep 22, 2023

roboquat merged commit 2529922 into main Sep 22, 2023
15 checks passed

roboquat deleted the at/default-branch-only branch September 22, 2023 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `prebuildDefaultBranchOnly` setting – EXP-574 #18716

Add `prebuildDefaultBranchOnly` setting – EXP-574 #18716

AlexTugarev commented Sep 14, 2023 •

edited

Loading

gtsiolis left a comment

AlexTugarev Sep 18, 2023

AlexTugarev Sep 18, 2023

AlexTugarev Sep 19, 2023

selfcontained left a comment

selfcontained Sep 19, 2023

svenefftinge Sep 21, 2023 •

edited

Loading

svenefftinge Sep 21, 2023

AlexTugarev Sep 21, 2023

svenefftinge Sep 21, 2023

svenefftinge Sep 21, 2023

svenefftinge Sep 21, 2023

AlexTugarev Sep 21, 2023

svenefftinge Sep 21, 2023

AlexTugarev Sep 21, 2023

AlexTugarev commented Sep 22, 2023

AlexTugarev commented Sep 22, 2023

Add prebuildDefaultBranchOnly setting – EXP-574 #18716

Add prebuildDefaultBranchOnly setting – EXP-574 #18716

Conversation

AlexTugarev commented Sep 14, 2023 • edited Loading

Description

🤖 Generated by Copilot at 2645ab6

Related Issue(s)

How to test

Documentation

Preview status

Build Options

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

selfcontained left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svenefftinge Sep 21, 2023 • edited Loading

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

AlexTugarev commented Sep 22, 2023

AlexTugarev commented Sep 22, 2023

Add `prebuildDefaultBranchOnly` setting – EXP-574 #18716

Add `prebuildDefaultBranchOnly` setting – EXP-574 #18716

AlexTugarev commented Sep 14, 2023 •

edited

Loading

`🤖 Generated by Copilot at 2645ab6`

svenefftinge Sep 21, 2023 •

edited

Loading