feat: mark prebuilds as such and set their preset ids by SasSwart · Pull Request #16965 · coder/coder

SasSwart · 2025-03-17T11:36:04Z

This pull request closes coder/internal#513

mtojek

Left a few comments, mainly related to naming convention.

mtojek · 2025-04-01T12:08:52Z

+		build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{
+			TemplateVersionID:       version.ID,
+			Transition:              codersdk.WorkspaceTransitionStart,
+			TemplateVersionPresetID: presets[0].ID,


What is ID here, UUID? Should we rely on a value that is human readable instead?

This ID is randomly generated by the database.

Let's say, I'm a template admin and want to modify the template with presets. I'm pushing a new template revision, it gets a new template version name and new template version ID. Does it happen the same for template version presets? will they get new IDs on every template version push, or will IDs be reused?

BTW we can move this to slack to keep things rolling.

Does it happen the same for template version presets?

Yes it does.
Presets are associated to a template version, so a new template version will create new preset entries.

mtojek · 2025-04-02T07:16:00Z

+		build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{
+			TemplateVersionID:       version.ID,
+			Transition:              codersdk.WorkspaceTransitionStart,
+			TemplateVersionPresetID: presets[0].ID,


Let's say, I'm a template admin and want to modify the template with presets. I'm pushing a new template revision, it gets a new template version name and new template version ID. Does it happen the same for template version presets? will they get new IDs on every template version push, or will IDs be reused?

BTW we can move this to slack to keep things rolling.

…t correctly

dannykopping

Generally LGTM, a few minor suggestions & nits
Thanks!

dannykopping · 2025-04-10T07:52:20Z

+		build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{
+			TemplateVersionID:       version.ID,
+			Transition:              codersdk.WorkspaceTransitionStart,
+			TemplateVersionPresetID: presets[0].ID,


Does it happen the same for template version presets?

Yes it does.
Presets are associated to a template version, so a new template version will create new preset entries.

dannykopping · 2025-04-10T07:53:56Z

+				Type: &proto.Response_Plan{
+					Plan: &proto.PlanComplete{
+						Presets: []*proto.Preset{{
+							Name: "test",


I'm wary of creating such minimal definitions here and in coderd/workspacebuilds_test.go (i.e. ones that do not have other fields like params); this does not reflect a legitimate use of presets.

Presets are allowed to have 0 parameters. This is to enable prebuilds on templates in which provisioning times are not contingent on parameters having certain values, which is a legitimate use case.

We're also not asserting anything that depends on these parameters in any way. I can add some, but I'm not sure what specifically to add that you find find beneficial? Should I just contrive a few parameters for good measure?

dannykopping · 2025-04-10T07:56:23Z

+
+		presets, err := client.TemplateVersionPresets(ctx, version.ID)
+		require.NoError(t, err)
+		require.Equal(t, 1, len(presets))


We're only ever testing a single preset, and I think there's some risk associated with that. Can you expand this, or add another test which uses multiple presets please?

dannykopping · 2025-04-10T08:06:19Z


 	// Log level changes the default logging verbosity of a provider ("info" if empty).
 	LogLevel ProvisionerLogLevel `json:"log_level,omitempty" validate:"omitempty,oneof=debug"`
+	// TemplateVersionPresetID is the ID of the template version preset to use for the build.


Nit: this comment is a bit redundant.

Suggested change

// TemplateVersionPresetID is the ID of the template version preset to use for the build.

// TemplateVersionPresetID is the ID of the selected preset, if available.

dannykopping · 2025-04-10T08:08:52Z

+	if metadata.GetIsPrebuild() {
+		env = append(env, provider.IsPrebuildEnvironmentVariable()+"=true")
+	}
+


On second thought, I'm thinking maybe we should always include the env for the absence of doubt:

Suggested change

if metadata.GetIsPrebuild() {

env = append(env, provider.IsPrebuildEnvironmentVariable()+"=true")

}

env = append(env, fmt.Sprintf("%s=%v", provider.IsPrebuildEnvironmentVariable(), metadata.GetIsPrebuild()))

github-actions Bot assigned SasSwart Mar 17, 2025

SasSwart force-pushed the 16930 branch from 16ce36c to 4f383f5 Compare March 17, 2025 11:57

SasSwart changed the title ~~mark prebuilds as such and set their preset ids~~ feat: mark prebuilds as such and set their preset ids Mar 17, 2025

SasSwart requested review from dannykopping and removed request for dannykopping March 17, 2025 11:58

dannykopping force-pushed the prebuilds-db branch from 7e062e2 to 51773ec Compare March 17, 2025 12:26

SasSwart mentioned this pull request Mar 17, 2025

Reintegrate Workspace Prebuilds feature branch into main coder/internal#516

Closed

12 tasks

SasSwart force-pushed the 16930 branch from 9ba7c2a to 7ff747e Compare March 18, 2025 06:19

SasSwart requested a review from johnstcn April 1, 2025 11:55

SasSwart marked this pull request as ready for review April 1, 2025 11:56

SasSwart requested a review from mtojek April 1, 2025 11:56

mtojek reviewed Apr 1, 2025

View reviewed changes

mtojek approved these changes Apr 2, 2025

View reviewed changes

Base automatically changed from prebuilds-db to main April 3, 2025 08:58

evgeniy-scherbina force-pushed the 16930 branch from 66c5ac1 to e354956 Compare April 3, 2025 21:15

SasSwart force-pushed the 16930 branch from 0ffc3d8 to 38ce101 Compare April 10, 2025 05:45

SasSwart added 6 commits April 10, 2025 06:30

mark prebuilds as such and set their preset ids

82e016c

add tests to ensure workspace builds that include a preset have it se…

c03ea52

…t correctly

test to ensure we mark prebuilds as such

3693d45

make -B gen fmt

beb814f

add template_version_preset_id to mock types

a5418ac

fix dbmem tests

f4f9b17

SasSwart force-pushed the 16930 branch from 38ce101 to f4f9b17 Compare April 10, 2025 06:49

go mod tidy && make -B gen

d11fd58

SasSwart requested a review from dannykopping April 10, 2025 07:53

dannykopping approved these changes Apr 10, 2025

View reviewed changes

dannykopping mentioned this pull request Apr 11, 2025

feat: implement reconciliation loop #17261

Merged

SasSwart added 2 commits April 11, 2025 12:42

deduplicate test and add back proto field

61a88e4

review notes

d8b74f6

SasSwart merged commit a986059 into main Apr 14, 2025

SasSwart deleted the 16930 branch April 14, 2025 13:34

github-actions Bot locked and limited conversation to collaborators Apr 14, 2025

	// TemplateVersionPresetID is the ID of the template version preset to use for the build.
	// TemplateVersionPresetID is the ID of the selected preset, if available.

Conversation

SasSwart commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannykopping left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SasSwart commented Mar 17, 2025 •

edited

Loading