- Notifications
You must be signed in to change notification settings - Fork907
chore: add dynamic parameter warning if missing metadata#17809
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
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
6f67a9a2d1701d0351ce5169bd5e9f947c3a0f4d58eb656ef478280b155f2cfFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.
Uh oh!
There was an error while loading.Please reload this page.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ALTER TABLE template_version_terraform_values DROP COLUMN provisionerd_version; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| ALTER TABLE template_version_terraform_values ADD COLUMN IF NOT EXISTS provisionerd_version TEXT NOT NULL DEFAULT ''; | ||
| COMMENT ON COLUMN template_version_terraform_values.provisionerd_version IS | ||
| 'What version of the provisioning engine was used to generate the cached plan and module files.'; |
Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.
Uh oh!
There was an error while loading.Please reload this page.
Some generated files are not rendered by default. Learn more abouthow customized files appear on GitHub.
Uh oh!
There was an error while loading.Please reload this page.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -8,9 +8,11 @@ import ( | ||
| "time" | ||
| "github.com/google/uuid" | ||
| "github.com/hashicorp/hcl/v2" | ||
| "golang.org/x/sync/errgroup" | ||
| "golang.org/x/xerrors" | ||
| "github.com/coder/coder/v2/apiversion" | ||
| "github.com/coder/coder/v2/coderd/database" | ||
| "github.com/coder/coder/v2/coderd/database/dbauthz" | ||
| "github.com/coder/coder/v2/coderd/files" | ||
| @@ -114,6 +116,9 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http | ||
| return | ||
| } | ||
| // If the err is sql.ErrNoRows, an empty terraform values struct is correct. | ||
| staticDiagnostics := parameterProvisionerVersionDiagnostic(tf) | ||
| owner, err := api.getWorkspaceOwnerData(ctx, user, templateVersion.OrganizationID) | ||
| if err != nil { | ||
| httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ | ||
| @@ -148,7 +153,7 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http | ||
| result, diagnostics := preview.Preview(ctx, input, templateFS) | ||
| response := codersdk.DynamicParametersResponse{ | ||
| ID: -1, | ||
| Diagnostics: previewtypes.Diagnostics(diagnostics.Extend(staticDiagnostics)), | ||
| } | ||
| if result != nil { | ||
| response.Parameters = result.Parameters | ||
| @@ -176,7 +181,7 @@ func (api *API) templateVersionDynamicParameters(rw http.ResponseWriter, r *http | ||
| result, diagnostics := preview.Preview(ctx, input, templateFS) | ||
| response := codersdk.DynamicParametersResponse{ | ||
| ID: update.ID, | ||
| Diagnostics: previewtypes.Diagnostics(diagnostics.Extend(staticDiagnostics)), | ||
| } | ||
| if result != nil { | ||
| response.Parameters = result.Parameters | ||
| @@ -269,3 +274,31 @@ func (api *API) getWorkspaceOwnerData( | ||
| Groups: groupNames, | ||
| }, nil | ||
| } | ||
| // parameterProvisionerVersionDiagnostic checks the version of the provisioner | ||
| // used to create the template version. If the version is less than 1.5, it | ||
| // returns a warning diagnostic. Only versions 1.5+ return the module & plan data | ||
| // required. | ||
| func parameterProvisionerVersionDiagnostic(tf database.TemplateVersionTerraformValue) hcl.Diagnostics { | ||
| missingMetadata := hcl.Diagnostic{ | ||
| Severity: hcl.DiagError, | ||
| Summary: "This template version is missing required metadata to support dynamic parameters. Go back to the classic creation flow.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. hmm. I don't like this. I would much prefer if we could have a way to expose this naturally to the frontend, forcing it over to the classic form. why make everyone click the "dynamic parameters suck" button? that's a super bad first impression There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. You are right, it is unfortunate. I want to get this in first, just to get an error up if someone toggles on dev and get this detection in. I will build on this experience. We just needsomething now, otherwise it fails silently. | ||
| Detail: "To restore full functionality, please re-import the terraform as a new template version.", | ||
| } | ||
| if tf.ProvisionerdVersion == "" { | ||
| return hcl.Diagnostics{&missingMetadata} | ||
| } | ||
| major, minor, err := apiversion.Parse(tf.ProvisionerdVersion) | ||
| if err != nil || tf.ProvisionerdVersion == "" { | ||
| return hcl.Diagnostics{&missingMetadata} | ||
| } else if major < 1 || (major == 1 && minor < 5) { | ||
| missingMetadata.Detail = "This template version does not support dynamic parameters. " + | ||
| "Some options may be missing or incorrect. " + | ||
| "Please contact an administrator to update the provisioner and re-import the template version." | ||
| return hcl.Diagnostics{&missingMetadata} | ||
| } | ||
| return nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| package coderd | ||
| import ( | ||
| "testing" | ||
| "github.com/stretchr/testify/require" | ||
| "github.com/coder/coder/v2/coderd/database" | ||
| ) | ||
| func Test_parameterProvisionerVersionDiagnostic(t *testing.T) { | ||
| t.Parallel() | ||
| testCases := []struct { | ||
| version string | ||
| warning bool | ||
| }{ | ||
| { | ||
| version: "", | ||
| warning: true, | ||
| }, | ||
| { | ||
| version: "invalid", | ||
| warning: true, | ||
| }, | ||
| { | ||
| version: "0.4", | ||
| warning: true, | ||
| }, | ||
| { | ||
| version: "0.5", | ||
| warning: true, | ||
| }, | ||
| { | ||
| version: "0.6", | ||
| warning: true, | ||
| }, | ||
| { | ||
| version: "1.4", | ||
| warning: true, | ||
| }, | ||
| { | ||
| version: "1.5", | ||
| warning: false, | ||
| }, | ||
| { | ||
| version: "1.6", | ||
| warning: false, | ||
| }, | ||
| { | ||
| version: "2.0", | ||
| warning: false, | ||
| }, | ||
| { | ||
| version: "2.5", | ||
| warning: false, | ||
| }, | ||
| { | ||
| version: "2.6", | ||
| warning: false, | ||
| }, | ||
| } | ||
| for _, tc := range testCases { | ||
| t.Run("Version_"+tc.version, func(t *testing.T) { | ||
| t.Parallel() | ||
| diags := parameterProvisionerVersionDiagnostic(database.TemplateVersionTerraformValue{ | ||
| ProvisionerdVersion: tc.version, | ||
| }) | ||
| if tc.warning { | ||
| require.Len(t, diags, 1, "expected warning") | ||
| } else { | ||
| require.Len(t, diags, 0, "expected no warning") | ||
| } | ||
| }) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -95,6 +95,7 @@ type Options struct { | ||||||||||||||||
| } | ||||||||||||||||
| type server struct { | ||||||||||||||||
| apiVersion string | ||||||||||||||||
| // lifecycleCtx must be tied to the API server's lifecycle | ||||||||||||||||
| // as when the API server shuts down, we want to cancel any | ||||||||||||||||
| // long-running operations. | ||||||||||||||||
| @@ -153,7 +154,9 @@ func (t Tags) Valid() error { | ||||||||||||||||
| return nil | ||||||||||||||||
| } | ||||||||||||||||
| func NewServer( | ||||||||||||||||
| lifecycleCtx context.Context, | ||||||||||||||||
| apiVersion string, | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I don't feel like coderd should be telling the provisioner what it's version is. This would break on deployments with external provisioners that are on an older version. and I know that's not really a thing we prioritize supporting, but this masks over the mismatched versions in a bad way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Coderd does not tell the provisioner. External provisioners call coderd via In that, they add a query param of their version: coder/enterprise/coderd/provisionerdaemons.go Lines 252 to 255 ineb656ef
Which is saved to the DB, and what we show on the provisioners page:
And in that serve function, that apiVersion is sent to the go routine that manages the gRPC connection:
So the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. The daemon client sends the version here: coder/codersdk/provisionerdaemons.go Line 273 ineb656ef
Idk why it sets it twice | ||||||||||||||||
| accessURL *url.URL, | ||||||||||||||||
| id uuid.UUID, | ||||||||||||||||
| organizationID uuid.UUID, | ||||||||||||||||
| @@ -214,6 +217,7 @@ func NewServer(lifecycleCtx context.Context, | ||||||||||||||||
| s := &server{ | ||||||||||||||||
| lifecycleCtx: lifecycleCtx, | ||||||||||||||||
| apiVersion: apiVersion, | ||||||||||||||||
| AccessURL: accessURL, | ||||||||||||||||
| ID: id, | ||||||||||||||||
| OrganizationID: organizationID, | ||||||||||||||||
| @@ -1536,10 +1540,11 @@ func (s *server) CompleteJob(ctx context.Context, completed *proto.CompletedJob) | ||||||||||||||||
| } | ||||||||||||||||
| err = s.Database.InsertTemplateVersionTerraformValuesByJobID(ctx, database.InsertTemplateVersionTerraformValuesByJobIDParams{ | ||||||||||||||||
| JobID: jobID, | ||||||||||||||||
| UpdatedAt: now, | ||||||||||||||||
| CachedPlan: plan, | ||||||||||||||||
| CachedModuleFiles: fileID, | ||||||||||||||||
| ProvisionerdVersion: s.apiVersion, | ||||||||||||||||
| }) | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| return nil, xerrors.Errorf("insert template version terraform data: %w", err) | ||||||||||||||||
Uh oh!
There was an error while loading.Please reload this page.