- Notifications
You must be signed in to change notification settings - Fork917
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
| @@ -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) | |||
There was a problem hiding this comment.
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.
| @@ -72,6 +72,9 @@ replace github.com/aquasecurity/trivy => github.com/coder/trivy v0.0.0-202505271 | |||
| // https://github.com/spf13/afero/pull/487 | |||
| replacegithub.com/spf13/afero =>github.com/aslilac/aferov0.0.0-20250403163713-f06e86036696 | |||
| // TODO: replace once we cut release. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
Uh oh!
There was an error while loading.Please reload this page.
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
There was a problem hiding this 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
| } | ||
| forname,tc:=rangecases { | ||
| tc:=tc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| tc := tc |
| t.Parallel() | ||
| // nolint:dogsled | ||
| _,filename,_,_:=runtime.Caller(0) |
There was a problem hiding this comment.
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?
| @@ -72,6 +72,9 @@ replace github.com/aquasecurity/trivy => github.com/coder/trivy v0.0.0-202505271 | |||
| // https://github.com/spf13/afero/pull/487 | |||
| replacegithub.com/spf13/afero =>github.com/aslilac/aferov0.0.0-20250403163713-f06e86036696 | |||
| // TODO: replace once we cut release. | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
| if(defaultPreset){ | ||
| // +1 because "None" is at index 0 | ||
| constdefaultIndex= | ||
| presets.findIndex((preset)=>preset.ID===defaultPreset.ID)+1; |
There was a problem hiding this comment.
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?
| @@ -125,6 +125,7 @@ export const PresetsButNoneSelected: Story = { | |||
| { | |||
| ID:"preset-1", | |||
| Name:"Preset 1", | |||
| Default:false, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
https://www.chromatic.com/test?appId=624de63c6aacee003aa84340&id=685406cf992466e39d7877ba
Label still shows preset 1 but the value in the form is that of preset 2.
Is this expected?
Uh oh!
There was an error while loading.Please reload this page.
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.
Default preset is autoselected on load: