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

fix: set preset parameters in the API rather than the frontend#17403

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

SasSwart merged 7 commits intomainfrom16930

Apr 16, 2025

Merged

fix: set preset parameters in the API rather than the frontend#17403

SasSwart merged 7 commits intomainfrom16930

Apr 16, 2025

Conversation

Copy link

Contributor

SasSwart commentedApr 15, 2025•
edited
Loading

Follow-up from aprevious Pull Request required some additional testing of Presets from the API perspective.

In the process of adding the new tests, I updated the API to enforce preset parameter values based on the selected preset instead of trusting whichever frontend makes the request. This avoids errors scenarios in prebuilds where a prebuild might expect a certain preset but find a different set of actual parameter values.

SasSwart added3 commits

April 15, 2025 11:00

fix: add some more thorough testing for preset workspace creation

19ec747

fix: set preset parameters in the API instead of relying on the frontend

2fad945

update expectations in TestWorkspaceBuildWithPreset

86d93c5

github-actionsbot assignedSasSwart

Apr 15, 2025

Merge branch 'main' into 16930

6601ce0

SasSwart requested review fromdannykopping andevgeniy-scherbina

April 15, 2025 14:32

dannykopping reviewed

Apr 15, 2025

View reviewed changes

coderd/workspaces_test.go OutdatedShow resolvedHide resolved

coderd/wsbuilder/wsbuilder_test.go Outdated

		@@ -960,6 +961,12 @@ func withInactiveVersion(params []database.TemplateVersionParameter) func(mTx *d
		}
		}

		func withTemplateVersionPreset(presetID uuid.UUID) func(mTx *dbmock.MockStore) {
		return func(mTx *dbmock.MockStore) {
		mTx.EXPECT().GetPresetParametersByPresetID(gomock.Any(), presetID).Return(nil, nil)

Copy link

Contributor

dannykoppingApr 15, 2025

Choose a reason for hiding this comment

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

I don't understand the value of returningnil, nil here; this won't execute your new codepath infindNewBuildParameterValue. Can you help me understand pls?

coderd/workspaces_test.go OutdatedShow resolvedHide resolved

simplify tests

24cb225

dannykopping reviewed

Apr 16, 2025

View reviewed changes

coderd/workspaces_test.go

		name: "Single Preset - No Preset Parameters But With Template Parameters",
		presets: []*proto.Preset{emptyPreset},
		templateVersionParameters: templateVersionParameters,
		selectedPresetIndex: ptr.Ref(0),

Copy link

Contributor

dannykoppingApr 16, 2025

Choose a reason for hiding this comment

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

What about instead just doing something like this?

Suggested change

	selectedPresetIndex:ptr.Ref(0),
	selectedPreset:&emptyPreset,

coderd/workspaces_test.go Outdated

		presets: []*proto.Preset{},
		templateVersionParameters: []*proto.RichParameter{},
		selectedPresetIndex: -1,
		templateVersionParameters: templateVersionParameters,