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

Add prebuildDefaultBranchOnly setting – EXP-574 #18716

Merged
merged 13 commits into from
Sep 22, 2023
Merged

Add prebuildDefaultBranchOnly setting – EXP-574 #18716

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8012bc7
add `prebuildDefaultBranchOnly` to `Project.settings`
AlexTugarev Sep 14, 2023
1f34982
evaluate prebuildDefaultBranchOnly in PrebuildManager
AlexTugarev Sep 14, 2023
5ce8104
add "Prebuild the default branch only" checkbox
AlexTugarev Sep 14, 2023
af8e407
fix missin PAPI mapping 🤯
AlexTugarev Sep 14, 2023
f17e190
fix PrebuildDefaultBranchOnly in gitpod-service.go
AlexTugarev Sep 15, 2023
4c814ee
turn text input into select input
AlexTugarev Sep 18, 2023
9a8e9df
update styles on project settings page
AlexTugarev Sep 18, 2023
623b5a5
mimic old behavior for pre-existing projects
AlexTugarev Sep 19, 2023
d53d9eb
use getPrebuildBranchStrategy in PrebuildManager
AlexTugarev Sep 19, 2023
9aac05a
add tests for PrebuildManager.prebuildPrecondition
AlexTugarev Sep 20, 2023
fc3e77d
fix getPrebuildEvents
AlexTugarev Sep 20, 2023
6ec7dae
explicit settings under the project should have precedence
AlexTugarev Sep 20, 2023
31a1409
render changes to project settings immediately
AlexTugarev Sep 22, 2023
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
42 changes: 34 additions & 8 deletions components/dashboard/src/projects/ProjectSettings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { useRefreshProjects } from "../data/projects/list-projects-query";
import { useToast } from "../components/toasts/Toasts";
import classNames from "classnames";
import { InputField } from "../components/forms/InputField";
import { SelectInputField } from "../components/forms/SelectInputField";

export function ProjectSettingsPage(props: { project?: Project; children?: React.ReactNode }) {
return (
Expand Down Expand Up @@ -92,6 +93,16 @@ export default function ProjectSettingsView() {
[project, setProject, toast],
);

const setPrebuildBranchStrategy = useCallback(
async (value: ProjectSettings.PrebuildBranchStrategy) => {
const prebuildDefaultBranchOnly = value === "defaultBranch";
await updateProjectSettings({
prebuildDefaultBranchOnly,
});
},
[updateProjectSettings],
);

const setWorkspaceClass = useCallback(
async (value: string) => {
if (!project) {
Expand Down Expand Up @@ -125,6 +136,8 @@ export default function ProjectSettingsView() {

const enablePrebuilds = Project.isPrebuildsEnabled(project);

const prebuildBranchStrategy = Project.getPrebuildBranchStrategy(project);

return (
<ProjectSettingsPage project={project}>
<Heading2>Project Name</Heading2>
Expand Down Expand Up @@ -168,14 +181,27 @@ export default function ProjectSettingsView() {
/>
{enablePrebuilds && (
AlexTugarev marked this conversation as resolved.
Show resolved Hide resolved
<>
<InputField label="Workspace machine type" disabled={!enablePrebuilds}>
<div className="max-w-md">
<SelectWorkspaceClassComponent
disabled={!enablePrebuilds}
selectedWorkspaceClass={project.settings?.workspaceClasses?.prebuild}
onSelectionChange={setWorkspaceClassForPrebuild}
/>
</div>
<SelectInputField
AlexTugarev marked this conversation as resolved.
Show resolved Hide resolved
disabled={!enablePrebuilds}
label="Build branches"
value={prebuildBranchStrategy}
AlexTugarev marked this conversation as resolved.
Show resolved Hide resolved
containerClassName="max-w-md ml-6 text-sm"
AlexTugarev marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Not sure if this is the most relevant line, but regarding the indentation, this ml-6 and mt-4.

Frame 1889

onChange={(val) => setPrebuildBranchStrategy(val as ProjectSettings.PrebuildBranchStrategy)}
>
<option value="defaultBranch">Default branch (e.g. main)</option>
Copy link
Contributor

@gtsiolis gtsiolis Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

issue(non-blocking): Not sure if we need the example branch name here, but we can remove at any time later.

<option value="allBranches">All branches</option>
{/* <option value="selectedBranches">Matched by pattern</option> */}
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be removed?

</SelectInputField>
<InputField
className="max-w-md ml-6 text-sm"
label="Workspace machine type"
disabled={!enablePrebuilds}
>
<SelectWorkspaceClassComponent
disabled={!enablePrebuilds}
selectedWorkspaceClass={project.settings?.workspaceClasses?.prebuild}
onSelectionChange={setWorkspaceClassForPrebuild}
/>
</InputField>
<CheckboxInputField
label="Enable Incremental Prebuilds"
Expand Down
1 change: 1 addition & 0 deletions components/dashboard/src/service/public-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ export function projectToProtocol(project: Project): ProtocolProject {
appInstallationId: "undefined",
settings: {
enablePrebuilds: project.settings?.prebuild?.enablePrebuilds,
prebuildDefaultBranchOnly: project.settings?.prebuild?.prebuildDefaultBranchOnly,
allowUsingPreviousPrebuilds: project.settings?.prebuild?.usePreviousPrebuilds,
keepOutdatedPrebuildsRunning: project.settings?.prebuild?.keepOutdatedPrebuildsRunning,
prebuildEveryNthCommit: project.settings?.prebuild?.prebuildEveryNth,
Expand Down
1 change: 1 addition & 0 deletions components/gitpod-protocol/go/gitpod-service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2274,6 +2274,7 @@ type Project struct {

type ProjectSettings struct {
EnablePrebuilds *bool `json:"enablePrebuilds,omitempty"`
PrebuildDefaultBranchOnly *bool `json:"prebuildDefaultBranchOnly,omitempty"`
UseIncrementalPrebuilds bool `json:"useIncrementalPrebuilds,omitempty"`
UsePersistentVolumeClaim bool `json:"usePersistentVolumeClaim,omitempty"`
KeepOutdatedPrebuildsRunning bool `json:"keepOutdatedPrebuildsRunning,omitempty"`
Expand Down
4 changes: 4 additions & 0 deletions components/gitpod-protocol/src/protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1366,6 +1366,10 @@ export namespace CommitContext {
}
return hasher.digest("hex");
}

export function isDefaultBranch(commitContext: CommitContext): boolean {
return commitContext.ref === commitContext.repository.defaultBranch;
}
}

export interface GitCheckoutInfo extends Commit {
Expand Down
32 changes: 30 additions & 2 deletions components/gitpod-protocol/src/teams-projects-protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ export interface ProjectConfig {

export interface ProjectSettings {
enablePrebuilds?: boolean;
/**
* Wether prebuilds (if enabled) should only be started on the default branch.
* Defaults to `true` on project creation.
*/
prebuildDefaultBranchOnly?: boolean;
Copy link
Member

@svenefftinge svenefftinge Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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 *?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

useIncrementalPrebuilds?: boolean;
keepOutdatedPrebuildsRunning?: boolean;
// whether new workspaces can start on older prebuilds and incrementally update
Expand All @@ -24,6 +29,9 @@ export interface ProjectSettings {
// preferred workspace classes
workspaceClasses?: WorkspaceClasses;
}
export namespace ProjectSettings {
export type PrebuildBranchStrategy = "defaultBranch" | "allBranches" | "selectedBranches";
}

export interface Project {
id: string;
Expand Down Expand Up @@ -67,11 +75,31 @@ export namespace Project {
// Defaulting to `true` for backwards compatibility. Ignoring non-boolean for `enablePrebuilds`
// for evaluation here allows to do any explicit migration of data or adjustment of the default
// behavior at a later point in time.
if (typeof project.settings?.enablePrebuilds === "undefined") {
if (!hasPrebuildSettings(project)) {
return true;
}

return project.settings.enablePrebuilds;
return !!project.settings?.enablePrebuilds;
}

export function hasPrebuildSettings(project: Project) {
return !(typeof project.settings?.enablePrebuilds === "undefined");
}

export function getPrebuildBranchStrategy(project: Project): ProjectSettings.PrebuildBranchStrategy {
if (!hasPrebuildSettings(project)) {
// returning "all branches" to mimic the default value of projects which were added
// before introduction of persisted settings for prebuilds.
return "allBranches";
}
if (typeof project.settings?.prebuildDefaultBranchOnly === "undefined") {
return "defaultBranch"; // default value for `settings.prebuildDefaultBranchOnly`
}
if (project.settings.prebuildDefaultBranchOnly) {
return "defaultBranch";
}
// TODO support "selectedBranches" next
return "allBranches";
}

export interface Overview {
Expand Down
1 change: 1 addition & 0 deletions components/public-api-server/pkg/apiv1/project.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ func projectSettingsToAPIResponse(s *protocol.ProjectSettings) *v1.ProjectSettin
return &v1.ProjectSettings{
Prebuild: &v1.PrebuildSettings{
EnablePrebuilds: s.EnablePrebuilds,
PrebuildDefaultBranchOnly: s.PrebuildDefaultBranchOnly,
EnableIncrementalPrebuilds: s.UseIncrementalPrebuilds,
KeepOutdatedPrebuildsRunning: s.KeepOutdatedPrebuildsRunning,
UsePreviousPrebuilds: s.AllowUsingPreviousPrebuilds,
Expand Down
1 change: 1 addition & 0 deletions components/public-api/gitpod/experimental/v1/projects.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ message PrebuildSettings {
bool use_previous_prebuilds = 3;
int32 prebuild_every_nth = 4;
optional bool enable_prebuilds = 5;
optional bool prebuild_default_branch_only = 6;
}

message WorkspaceSettings {
Expand Down
Loading