NotificationsYou must be signed in to change notification settings
Fork925
Star10.1k

feat: allow for default presets#18445

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

dannykopping merged 13 commits intomainfromdk/default-preset

Jun 24, 2025

Merged

feat: allow for default presets#18445

dannykopping merged 13 commits intomainfromdk/default-preset

Jun 24, 2025

Conversation

Copy link

Contributor

dannykopping commentedJun 19, 2025•
edited
Loading

Disclaimer: mostly implemented by Claude Code.

Depends on:coder/terraform-provider-coder#414
Closes:#18110

I've tested this against both the classic and new (dynamic parameters) approaches.

...data"coder_workspace_preset""goland" {default=true# <-----------------name="I Like GoLand"parameters={"jetbrains_ide"="GO""cpu"="2"  }}data"coder_workspace_preset""python" {name="Some Like PyCharm"parameters={"jetbrains_ide"="PY"  }}...

Default preset is autoselected on load:

dannykopping added4 commits

June 18, 2025 16:35

chore: database changes

89da41b

Signed-off-by: Danny Kopping <dannykopping@gmail.com>

chore: proto changes

7e75433

Signed-off-by: Danny Kopping <dannykopping@gmail.com>

chore: use new provider, accept values for default

0fb3743

Signed-off-by: Danny Kopping <dannykopping@gmail.com>

feat: full implementation

e9a218c

Signed-off-by: Danny Kopping <dannykopping@gmail.com>

github-actionsbot assigneddannykopping

Jun 19, 2025

dannykopping changed the title~~feat: default presets~~feat: allow for default presets

Jun 19, 2025

chore: improve version constraints

a6e4d0a

Signed-off-by: Danny Kopping <dannykopping@gmail.com>

dannykopping commented

Jun 19, 2025

View reviewed changes

CLAUDE.md

		@@ -103,5 +103,4 @@ Read [cursor rules](.cursorrules).

		The frontend is contained in the site folder.

		For building Frontend refer to [this document](docs/contributing/frontend.md)

Copy link

ContributorAuthor

dannykoppingJun 19, 2025

Choose a reason for hiding this comment

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

Drive-by: link is dead.

Copy link

Member

aslilacJun 23, 2025•
edited
Loading

Choose a reason for hiding this comment

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

~~it's in docs/about/contributing/ now. can you leave this as is and I'll do a quick pr to fix the link?~~

sorry, I just looked at the whole file and this diff makes more sense now. the line got duplicated, with one having the out of date path and one having the correct path. you're just removing the stale one.

go.mod

		@@ -72,6 +72,9 @@ replace github.com/aquasecurity/trivy => github.com/coder/trivy v0.0.0-202505271
		// https://github.com/spf13/afero/pull/487
		replace github.com/spf13/afero => github.com/aslilac/afero v0.0.0-20250403163713-f06e86036696

		// TODO: replace once we cut release.

Copy link

ContributorAuthor

dannykoppingJun 19, 2025

Choose a reason for hiding this comment

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

TODO: depends oncoder/terraform-provider-coder#414.

Copy link

Member

johnstcnJun 19, 2025

Choose a reason for hiding this comment

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

Might make sense to wait until that is merged + tagged first?

dannykopping requested review fromSasSwart andBrunoQuaresma

June 19, 2025 08:27

dannykopping marked this pull request as ready for review

June 19, 2025 08:27

dannykopping requested review fromspikecurtis andjohnstcn ascode owners

June 19, 2025 08:27

johnstcn reviewed

Jun 19, 2025

View reviewed changes

provisionersdk/proto/provisioner.proto OutdatedShow resolvedHide resolved

chore: update provisioner version comment

1b737ff

Signed-off-by: Danny Kopping <dannykopping@gmail.com>

johnstcn approved these changes

Jun 19, 2025

View reviewed changes

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

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

Changes look OK to me, with the provisio that we should merge + pre-release the provider change first

provisioner/terraform/resources_test.go

		}

		for name, tc := range cases {
		tc := tc

Copy link

Member

johnstcnJun 19, 2025

Choose a reason for hiding this comment

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

Suggested change

tc := tc

provisioner/terraform/resources_test.go

		t.Parallel()

		// nolint:dogsled
		_, filename, _, _ := runtime.Caller(0)

Copy link

Member

johnstcnJun 19, 2025

Choose a reason for hiding this comment

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

nit, non-blocking: I understand that this is copied from other tests, but I wonder if we could uset.Name() instead?

go.mod

		@@ -72,6 +72,9 @@ replace github.com/aquasecurity/trivy => github.com/coder/trivy v0.0.0-202505271
		// https://github.com/spf13/afero/pull/487
		replace github.com/spf13/afero => github.com/aslilac/afero v0.0.0-20250403163713-f06e86036696

		// TODO: replace once we cut release.

Copy link

Member

johnstcnJun 19, 2025

Choose a reason for hiding this comment

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

Might make sense to wait until that is merged + tagged first?

coderd/database/migrations/000338_template_version_preset_default.up.sql Outdated

Copy link

Member

johnstcnJun 19, 2025

Choose a reason for hiding this comment

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

Obligatory reminder to check migration number before merge

chore: generate test fixtures correctly

f0f77fb

Signed-off-by: Danny Kopping <dannykopping@gmail.com>

SasSwart reviewed

Jun 19, 2025

View reviewed changes

site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx Outdated

		if (defaultPreset) {
		// +1 because "None" is at index 0
		const defaultIndex =
		presets.findIndex((preset) => preset.ID === defaultPreset.ID) + 1;

Copy link

Contributor

SasSwartJun 19, 2025

Choose a reason for hiding this comment

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

It looks like this fine, but I've been caught byfindIndex before while working on presets. Is there any way thatfindIndex might return -1 here?

SasSwart reviewed

Jun 19, 2025

View reviewed changes

site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.stories.tsxShow resolvedHide resolved

dannykopping added2 commits

June 23, 2025 21:45

Merge branch 'main' of github.com:/coder/coder into dk/default-preset

83192a7

refactor: improve preset selection logic in workspace creation

0d9d751

Replace findIndex with find() to avoid -1 edge case and consolidate preset initialization into single useEffect for better maintainability.Signed-off-by: Danny Kopping <dannykopping@gmail.com>

aslilac reviewed

Jun 23, 2025

View reviewed changes

site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsx

		setPresetOptions(options);
		const defaultPreset = presets.find((p) => p.Default);
		if (defaultPreset) {
		const idx = presets.indexOf(defaultPreset) + 1; // +1 for "None"

Copy link

Member

aslilacJun 23, 2025

Choose a reason for hiding this comment

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

this is really confusing. so0 is "none", but we want to special case it to1, which should be the default ifdefaultPreset is true? is the first "real" option actually guaranteed to be the default? if so, why are we searching the array for it?

Copy link

ContributorAuthor

dannykoppingJun 24, 2025

Choose a reason for hiding this comment

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

None is index 0,indexOf can return -1.

site/src/pages/CreateWorkspacePage/CreateWorkspacePageView.tsxShow resolvedHide resolved

site/src/pages/CreateWorkspacePage/CreateWorkspacePageViewExperimental.tsx

		if (defaultPreset) {
		// +1 because "None" is at index 0
		const defaultIndex =
		presets.findIndex((preset) => preset.ID === defaultPreset.ID) + 1;

Copy link

Member

aslilacJun 23, 2025

Choose a reason for hiding this comment

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

...or I guess it's because everything is thispresets array ends up getting offset by one for having"none" at the beginning? that feels really weird, it'd make a lot more sense to me if the numbers were preserved and"none" was represented bynull or something

Copy link

ContributorAuthor

dannykoppingJun 24, 2025

Choose a reason for hiding this comment

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

Agreed; I think we need a wider refactor of the implementation. For now though, I'm going to have to use what's present.@SasSwart WDYT?

Copy link

Contributor

SasSwartJun 24, 2025

Choose a reason for hiding this comment

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

Coping with the none preset in the FE has felt weird from the start. I'm considering the idea that "None" should be a preset returned like any other from the endpoint that lists presets. it would simplify the FE quite a bit and centralize how me manage it.

dannykopping added2 commits

June 24, 2025 11:12

Merge branch 'main' of github.com:coder/coder into dk/default-preset

590eb4e

chore: fix up storybook tests

737e1c6

Signed-off-by: Danny Kopping <danny@coder.com>

dannykopping force-pushed thedk/default-preset branch from9bb383a to737e1c6Compare

June 24, 2025 09:24

dannykopping added2 commits

June 24, 2025 11:56

Merge branch 'main' of github.com:/coder/coder into dk/default-preset

ce92f1f

chore: ensure "None" value is set correctly

371ac5a

Signed-off-by: Danny Kopping <dannykopping@gmail.com>

dannykopping merged commit6cc4cfa intomain

Jun 24, 2025

39 checks passed

dannykopping deleted the dk/default-preset branch

June 24, 2025 10:19

github-actionsbot locked and limited conversation to collaborators

Jun 24, 2025

Labels

None yet

4 participants

Movatterモバイル変換

feat: allow for default presets#18445

feat: allow for default presets#18445

Uh oh!

Conversation

dannykopping commentedJun 19, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aslilacJun 23, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

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!

johnstcn left a comment

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!

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!

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!

Uh oh!

dannykopping commentedJun 19, 2025•
edited
Loading

aslilacJun 23, 2025•
edited
Loading