Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

chore: remove dynamic-parameters experiment#18290

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

Open
jaaydenh wants to merge17 commits intomain
base:main
Choose a base branch
Loading
fromjaaydenh/remove-dynamic-params-experiment

Conversation

jaaydenh
Copy link
Contributor

No description provided.

@jaaydenhjaaydenh self-assigned thisJun 9, 2025
jaaydenhand others added5 commitsJune 9, 2025 14:47
- Regenerated TypeScript types to remove dynamic-parameters experiment- Updated useDynamicParametersOptOut hook to always return optedOut: true  since the experiment was removed- This fixes test failures related to dynamic parameters functionalityCo-authored-by: jaaydenh <1858163+jaaydenh@users.noreply.github.com>
@blink-soblink.so
Copy link
Contributor

Fix for Test Failures

I've identified and fixed the test failures in this PR. The issues were:

1.TestPatchTemplateMeta/ClassicParameterFlow

The test was expectingUseClassicParameterFlow to befalse by default, but the new migration (000334_dynamic_parameters_opt_out.up.sql) sets it totrue. I updated the test assertion to expect the correct default value.

2.TestDynamicParametersWithTerraformValues/RebuildParameters

The test was expecting dynamic parameters to work, but withuse_classic_parameter_flow now defaulting totrue, dynamic parameters are disabled by default. I updated the test setup to explicitly enable dynamic parameters by settingUseClassicParameterFlow tofalse for the test templates.

Changes Made:

  • Updatedcoderd/templates_test.go line 1550 to expecttrue instead offalse
  • Updatedcoderd/parameters_test.go to explicitly disable classic parameter flow in tests that require dynamic parameters
  • Fixed variable shadowing issues introduced by the changes

The fix has been pushed to a separate branch:https://github.com/coder/coder/tree/pr-18290

These changes ensure the tests align with the new default behavior where templates opt out of dynamic parameters by default.

@blink-soblink.so
Copy link
Contributor

Additional Fix for wsbuilder Test Failure

I've fixed the additional test failure inTestWorkspaceBuildWithRichParameters/StartWorkspaceWithLegacyParameterValues.

The Issue:

The test was expectingGetTemplateVersionTerraformValues to be called (viawithTerraformValuesErrNoRows), but this call was not happening because the template hadUseClassicParameterFlow: true.

The Fix:

  1. OptimizedusingDynamicParameters() function inwsbuilder.go:

    • Reordered the checks to avoid unnecessary database calls
    • Now checksUseClassicParameterFlow first before attempting to fetch terraform values
    • This is more efficient and prevents the test failure
  2. Updated all affected tests inwsbuilder_test.go:

    • RemovedwithTerraformValuesErrNoRows expectations from tests wherewithTemplate setsUseClassicParameterFlow: true
    • This aligns the test expectations with the actual behavior

The changes ensure that when classic parameter flow is enabled, we don't make unnecessary database calls to fetch terraform values.

@jaaydenhjaaydenh requested a review fromEmyrkJune 11, 2025 09:39
@@ -249,6 +249,7 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) {
Value:"GO",
},
}
request.EnableDynamicParameters=true
Copy link
Member

Choose a reason for hiding this comment

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

We should really set the template to dynamic params, because we are going to drop this field soon.

Copy link
Member

Choose a reason for hiding this comment

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

I changed it here:414801d

Comment on lines 1044 to 1067
func (b*Builder)usingDynamicParameters()bool {
vals,err:=b.getTemplateTerraformValues()
ifb.dynamicParametersEnabled!=nil {
return*b.dynamicParametersEnabled
}

tpl,err:=b.getTemplate()
iferr!=nil {
returnfalse// Let another part of the code get this error
}
iftpl.UseClassicParameterFlow {
returnfalse
}

if!ProvisionerVersionSupportsDynamicParameters(vals.ProvisionerdVersion) {
vals,err:=b.getTemplateTerraformValues()
iferr!=nil {
returnfalse
}

ifb.dynamicParametersEnabled!=nil {
return*b.dynamicParametersEnabled
if!ProvisionerVersionSupportsDynamicParameters(vals.ProvisionerdVersion) {
returnfalse
}

tpl,err:=b.getTemplate()
iferr!=nil {
returnfalse// Let another part of the code get this error
}
return!tpl.UseClassicParameterFlow
returntrue
}
Copy link
Member

Choose a reason for hiding this comment

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

👍

@jaaydenhjaaydenh marked this pull request as ready for reviewJune 11, 2025 20:24
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@EmyrkEmyrkEmyrk left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@jaaydenhjaaydenh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@jaaydenh@Emyrk

[8]ページ先頭

©2009-2025 Movatter.jp