fix: template version replacement & metadata updates

Author: ethanndickson

docs/resources/template.md

@@ -55,7 +55,7 @@ Optional:
 
 - `active` (Boolean) Whether this version is the active version of the template. Only one version can be active at a time.
 - `message` (String) A message describing the changes in this version of the template. Messages longer than 72 characters will be truncated.
-- `name` (String) The name of the template version. Automatically generated if not provided.
+- `name` (String) The name of the template version. Automatically generated if not provided. If provided, the name *must* change each time the directory contents are updated.
 - `provisioner_tags` (Attributes Set) Provisioner tags for the template version. (see [below for nested schema](#nestedatt--versions--provisioner_tags))
 - `tf_vars` (Attributes Set) Terraform variables for the template version. (see [below for nested schema](#nestedatt--versions--tf_vars))
 

internal/provider/template_resource.go

@@ -3,6 +3,7 @@ package provider
 import (
 	"bufio"
 	"context"
+	"encoding/json"
 	"fmt"
 	"io"
 
@@ -346,7 +347,7 @@ func (r *TemplateResource) Schema(ctx context.Context, req resource.SchemaReques
 							Computed:   true,
 						},
 						"name": schema.StringAttribute{
-							MarkdownDescription: "The name of the template version. Automatically generated if not provided.",
+							MarkdownDescription: "The name of the template version. Automatically generated if not provided. If provided, the name *must* change each time the directory contents are updated.",
 							Optional:            true,
 							Computed:            true,
 						},
@@ -502,6 +503,17 @@ func (r *TemplateResource) Create(ctx context.Context, req resource.CreateReques
 	data.ID = UUIDValue(templateResp.ID)
 	data.DisplayName = types.StringValue(templateResp.DisplayName)
 
+	// We have to init the private state again since the PlanModifyObject private
+	// state is not accessible in Create
+	resp.Diagnostics.Append(setEmptyPrivateState(ctx, resp.Private)...)
+	if resp.Diagnostics.HasError() {
+		return
+	}
+	resp.Diagnostics.Append(data.Versions.writePrivateState(ctx, resp.Private)...)
+	if resp.Diagnostics.HasError() {
+		return
+	}
+
 	// Save data into Terraform sutate
 	resp.Diagnostics.Append(resp.State.Set(ctx, &data)...)
 }
@@ -569,11 +581,11 @@ func (r *TemplateResource) Read(ctx context.Context, req resource.ReadRequest, r
 }
 
 func (r *TemplateResource) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) {
-	var planState TemplateResourceModel
+	var newState TemplateResourceModel
 	var curState TemplateResourceModel
 
 	// Read Terraform plan data into the model
-	resp.Diagnostics.Append(req.Plan.Get(ctx, &planState)...)
+	resp.Diagnostics.Append(req.Plan.Get(ctx, &newState)...)
 
 	if resp.Diagnostics.HasError() {
 		return
@@ -585,25 +597,25 @@ func (r *TemplateResource) Update(ctx context.Context, req resource.UpdateReques
 		return
 	}
 
-	if planState.OrganizationID.IsUnknown() {
-		planState.OrganizationID = UUIDValue(r.data.DefaultOrganizationID)
+	if newState.OrganizationID.IsUnknown() {
+		newState.OrganizationID = UUIDValue(r.data.DefaultOrganizationID)
 	}
 
-	if planState.DisplayName.IsUnknown() {
-		planState.DisplayName = planState.Name
+	if newState.DisplayName.IsUnknown() {
+		newState.DisplayName = newState.Name
 	}
 
-	orgID := planState.OrganizationID.ValueUUID()
+	orgID := newState.OrganizationID.ValueUUID()
 
-	templateID := planState.ID.ValueUUID()
+	templateID := newState.ID.ValueUUID()
 
 	client := r.data.Client
 
-	templateMetadataChanged := !planState.EqualTemplateMetadata(curState)
+	templateMetadataChanged := !newState.EqualTemplateMetadata(curState)
 	// This is required, as the API will reject no-diff updates.
 	if templateMetadataChanged {
 		tflog.Trace(ctx, "change in template metadata detected, updating.")
-		updateReq := planState.toUpdateRequest(ctx, resp)
+		updateReq := newState.toUpdateRequest(ctx, resp)
 		if resp.Diagnostics.HasError() {
 			return
 		}
@@ -618,9 +630,9 @@ func (r *TemplateResource) Update(ctx context.Context, req resource.UpdateReques
 
 	// Since the everyone group always gets deleted by `DisableEveryoneGroupAccess`, we need to run this even if there
 	// were no ACL changes but the template metadata was updated.
-	if !planState.ACL.IsNull() && (!curState.ACL.Equal(planState.ACL) || templateMetadataChanged) {
+	if !newState.ACL.IsNull() && (!curState.ACL.Equal(newState.ACL) || templateMetadataChanged) {
 		var acl ACL
-		resp.Diagnostics.Append(planState.ACL.As(ctx, &acl, basetypes.ObjectAsOptions{})...)
+		resp.Diagnostics.Append(newState.ACL.As(ctx, &acl, basetypes.ObjectAsOptions{})...)
 		if resp.Diagnostics.HasError() {
 			return
 		}
@@ -632,51 +644,64 @@ func (r *TemplateResource) Update(ctx context.Context, req resource.UpdateReques
 		tflog.Trace(ctx, "successfully updated template ACL")
 	}
 
-	for idx, plannedVersion := range planState.Versions {
-		var curVersionID uuid.UUID
-		// All versions in the state are guaranteed to have known IDs
-		foundVersion := curState.Versions.ByID(plannedVersion.ID)
-		// If the version is new, or if the directory hash has changed, create a new version
-		if foundVersion == nil || foundVersion.DirectoryHash != plannedVersion.DirectoryHash {
+	for idx := range newState.Versions {
+		if newState.Versions[idx].ID.IsUnknown() {
 			tflog.Trace(ctx, "discovered a new or modified template version")
-			versionResp, err := newVersion(ctx, client, newVersionRequest{
-				Version:        &plannedVersion,
+			uploadResp, err := newVersion(ctx, client, newVersionRequest{
+				Version:        &newState.Versions[idx],
 				OrganizationID: orgID,
 				TemplateID:     &templateID,
 			})
 			if err != nil {
 				resp.Diagnostics.AddError("Client Error", err.Error())
 				return
 			}
-			curVersionID = versionResp.ID
+			versionResp, err := client.TemplateVersion(ctx, uploadResp.ID)
+			if err != nil {
+				resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Failed to get template version: %s", err))
+				return
+			}
+			newState.Versions[idx].ID = UUIDValue(versionResp.ID)
+			newState.Versions[idx].Name = types.StringValue(versionResp.Name)
 		} else {
-			// Or if it's an existing version, get the ID
-			curVersionID = plannedVersion.ID.ValueUUID()
-		}
-		versionResp, err := client.TemplateVersion(ctx, curVersionID)
-		if err != nil {
-			resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Failed to get template version: %s", err))
-			return
+			versionResp, err := client.UpdateTemplateVersion(ctx, newState.Versions[idx].ID.ValueUUID(), codersdk.PatchTemplateVersionRequest{
+				Name:    newState.Versions[idx].Name.ValueString(),
+				Message: newState.Versions[idx].Message.ValueStringPointer(),
+			})
+			if err != nil {
+				resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Failed to update template version metadata: %s", err))
+				return
+			}
+			// If the name was not provided on an update we set it to the patch result, which is the previous name.
+			// There's no way to go back to an auto-generated name unless the template version files itself change.
+			newState.Versions[idx].Name = types.StringValue(versionResp.Name)
 		}
-		if plannedVersion.Active.ValueBool() {
+		if newState.Versions[idx].Active.ValueBool() {
 			tflog.Trace(ctx, "marking template version as active", map[string]any{
-				"version_id":  versionResp.ID,
-				"template_id": templateID,
+				"version_id":  newState.Versions[idx].ID.ValueString(),
+				"template_id": templateID.String(),
 			})
 			err := client.UpdateActiveTemplateVersion(ctx, templateID, codersdk.UpdateActiveTemplateVersion{
-				ID: versionResp.ID,
+				ID: newState.Versions[idx].ID.ValueUUID(),
 			})
 			if err != nil {
 				resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Failed to update active template version: %s", err))
 				return
 			}
 			tflog.Trace(ctx, "marked template version as active")
 		}
-		planState.Versions[idx].ID = UUIDValue(versionResp.ID)
+	}
+
+	// We only want the previous apply in the state at any given time
+	resp.Diagnostics.Append(setEmptyPrivateState(ctx, resp.Private)...)
+
+	resp.Diagnostics.Append(newState.Versions.writePrivateState(ctx, resp.Private)...)
+	if resp.Diagnostics.HasError() {
+		return
 	}
 
 	// Save updated data into Terraform state
-	resp.Diagnostics.Append(resp.State.Set(ctx, &planState)...)
+	resp.Diagnostics.Append(resp.State.Set(ctx, &newState)...)
 }
 
 func (r *TemplateResource) Delete(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) {
@@ -766,25 +791,26 @@ func (d *directoryHashPlanModifier) MarkdownDescription(context.Context) string
 
 // PlanModifyObject implements planmodifier.Object.
 func (d *directoryHashPlanModifier) PlanModifyObject(ctx context.Context, req planmodifier.ObjectRequest, resp *planmodifier.ObjectResponse) {
-	attributes := req.PlanValue.Attributes()
-	directory, ok := attributes["directory"].(types.String)
-	if !ok {
-		resp.Diagnostics.AddError("Client Error", fmt.Sprintf("unexpected type for directory, got: %T", directory))
+	var data TemplateVersion
+	resp.Diagnostics.Append(req.PlanValue.As(ctx, &data, basetypes.ObjectAsOptions{})...)
+	if resp.Diagnostics.HasError() {
 		return
 	}
 
-	hash, err := computeDirectoryHash(directory.ValueString())
+	hash, err := computeDirectoryHash(data.Directory.ValueString())
 	if err != nil {
 		resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Failed to compute directory hash: %s", err))
 		return
 	}
-	attributes["directory_hash"] = types.StringValue(hash)
-	out, diag := types.ObjectValue(req.PlanValue.AttributeTypes(ctx), attributes)
-	if diag.HasError() {
-		resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Failed to create plan object: %s", diag))
+
+	data.DirectoryHash = types.StringValue(hash)
+	// Populate version IDs or mark them as unknown if the hash has changed
+	resp.Diagnostics.Append(data.readFromPrivateState(ctx, req.Private)...)
+	if resp.Diagnostics.HasError() {
 		return
 	}
-	resp.PlanValue = out
+
+	resp.PlanValue, resp.Diagnostics = types.ObjectValueFrom(ctx, req.PlanValue.AttributeTypes(ctx), data)
 }
 
 func NewDirectoryHashPlanModifier() planmodifier.Object {
@@ -1062,3 +1088,81 @@ func (r *TemplateResourceModel) toCreateRequest(ctx context.Context, resp *resou
 		DisableEveryoneGroupAccess:     !r.ACL.IsNull(),
 	}
 }
+
+type LastVersionsByHash = map[string]PreviousTemplateVersion
+
+var LastVersionsKey = "last_versions"
+
+type PreviousTemplateVersion struct {
+	ID   uuid.UUID `json:"id"`
+	Name string    `json:"name"`
+}
+
+type privateState interface {
+	GetKey(ctx context.Context, key string) ([]byte, diag.Diagnostics)
+	SetKey(ctx context.Context, key string, value []byte) diag.Diagnostics
+}
+
+func (v Versions) writePrivateState(ctx context.Context, ps privateState) (diags diag.Diagnostics) {
+	var lv LastVersionsByHash
+	lvBytes, diag := ps.GetKey(ctx, LastVersionsKey)
+	if diag.HasError() {
+		return diag
+	}
+	err := json.Unmarshal(lvBytes, &lv)
+	if err != nil {
+		diags.AddError("Client Error", fmt.Sprintf("Failed to unmarshal private state when writing: %s", err))
+		return diags
+	}
+	for _, version := range v {
+		lv[version.DirectoryHash.ValueString()] = PreviousTemplateVersion{
+			ID:   version.ID.ValueUUID(),
+			Name: version.ID.ValueString(),
+		}
+		lvBytes, err = json.Marshal(lv)
+		if err != nil {
+			diags.AddError("Client Error", fmt.Sprintf("Failed to marshal private state: %s", err))
+			return diags
+		}
+	}
+	return ps.SetKey(ctx, LastVersionsKey, lvBytes)
+}
+
+func (v *TemplateVersion) readFromPrivateState(ctx context.Context, ps privateState) (diags diag.Diagnostics) {
+	var lv LastVersionsByHash
+	lvBytes, diag := ps.GetKey(ctx, LastVersionsKey)
+	if diag.HasError() {
+		diags.Append(diag...)
+		return
+	}
+	// If this is the first read, init the private state value
+	if lvBytes == nil {
+		setEmptyPrivateState(ctx, ps)
+		return
+	}
+	err := json.Unmarshal(lvBytes, &lv)
+	if err != nil {
+		diags.AddError("Client Error", fmt.Sprintf("Failed to unmarshal private state when reading: %s", err))
+		return
+	}
+
+	prev, ok := lv[v.DirectoryHash.ValueString()]
+	// If not in state, mark as known after apply since we'll create a new version.
+	// Versions who's Terraform configuration has not changed will have known
+	// IDs at this point, so we need to set this manually.
+	if !ok {
+		v.ID = NewUUIDUnknown()
+		return
+	}
+	// Otherwise, use the existing ID for this hash
+	v.ID = UUIDValue(prev.ID)
+	return
+}
+
+func setEmptyPrivateState(ctx context.Context, ps privateState) (diags diag.Diagnostics) {
+	pvBytes, err := json.Marshal(make(LastVersionsByHash))
+	if err != nil {
+		panic("failed to marshal empty private state")
+	}
+	return ps.SetKey(ctx, LastVersionsKey, pvBytes)
+}

internal/provider/template_resource_test.go

@@ -2,6 +2,7 @@ package provider
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"regexp"
 	"slices"
@@ -29,7 +30,7 @@ func TestAccTemplateResource(t *testing.T) {
 		Name:  PtrTo("example-template"),
 		Versions: []testAccTemplateVersionConfig{
 			{
-				Name:      PtrTo("main"),
+				// Auto-generated version name
 				Directory: PtrTo("../../integration/template-test/example-template/"),
 				Active:    PtrTo(true),
 				// TODO(ethanndickson): Remove this when we add in `*.tfvars` parsing
@@ -102,6 +103,7 @@ func TestAccTemplateResource(t *testing.T) {
 		IsUnitTest:               true,
 		ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
 		Steps: []resource.TestStep{
+			// Init, creates the first version
 			{
 				Config: cfg1.String(t),
 				Check: resource.ComposeTestCheckFunc(
@@ -123,23 +125,46 @@ func TestAccTemplateResource(t *testing.T) {
 					resource.TestCheckResourceAttr("coderd_template.test", "time_til_dormant_autodelete_ms", "0"),
 					resource.TestCheckResourceAttr("coderd_template.test", "require_active_version", "false"),
 					resource.TestMatchTypeSetElemNestedAttrs("coderd_template.test", "versions.*", map[string]*regexp.Regexp{
-						"name":           regexp.MustCompile("main"),
-						"id":             regexp.MustCompile(".*"),
+						"name":           regexp.MustCompile(".+"),
+						"id":             regexp.MustCompile(".+"),
 						"directory_hash": regexp.MustCompile(".+"),
 						"message":        regexp.MustCompile(""),
 					}),
 				),
 			},
+			// Modify template contents. Creates a second version.
+			{
+				Config: cfg1.String(t),
+				PreConfig: func() {
+					file := fmt.Sprintf("%s/terraform.tfvars", *cfg1.Versions[0].Directory)
+					newFile := []byte("name = \"world2\"")
+					err := os.WriteFile(file, newFile, 0644)
+					require.NoError(t, err)
+				},
+				// Version should be updated, checked at the end
+			},
+			// Undo modification. Creates a third version since it differs from the last apply
+			{
+				Config: cfg1.String(t),
+				PreConfig: func() {
+					file := fmt.Sprintf("%s/terraform.tfvars", *cfg1.Versions[0].Directory)
+					newFile := []byte("name = \"world\"")
+					err := os.WriteFile(file, newFile, 0644)
+					require.NoError(t, err)
+				},
+				// Version should be updated, checked at the end
+			},
 			// Import
 			{
 				Config:            cfg1.String(t),
 				ResourceName:      "coderd_template.test",
 				ImportState:       true,
 				ImportStateVerify: true,
 				// In the real world, `versions` needs to be added to the configuration after importing
+				// We can't import ACL as we can't currently differentiate between managed and unmanaged ACL
 				ImportStateVerifyIgnore: []string{"versions", "acl"},
 			},
-			// Update existing version & metadata
+			// Change existing version directory & name, update template metadata. Creates a fourth version.
 			{
 				Config: cfg2.String(t),
 				Check: resource.ComposeAggregateTestCheckFunc(
@@ -153,7 +178,7 @@ func TestAccTemplateResource(t *testing.T) {
 					}),
 				),
 			},
-			// Append version
+			// Append version. Creates a fifth version
 			{
 				Config: cfg3.String(t),
 				Check: resource.ComposeAggregateTestCheckFunc(
@@ -181,7 +206,7 @@ func TestAccTemplateResource(t *testing.T) {
 					}),
 				),
 			},
-			// Swap versions
+			// Swap versions in-place
 			{
 				Config: cfg5.String(t),
 				Check: resource.ComposeAggregateTestCheckFunc(
@@ -214,6 +239,21 @@ func TestAccTemplateResource(t *testing.T) {
 					resource.TestCheckNoResourceAttr("coderd_template.test", "acl"),
 				),
 			},
+			// Count orphaned versions
+			{
+				Config: cfg7.String(t),
+				PreConfig: func() {
+					templates, err := client.Templates(ctx)
+					require.NoError(t, err)
+					require.Len(t, templates, 1)
+					versions, err := client.TemplateVersionsByTemplate(ctx, codersdk.TemplateVersionsByTemplateRequest{
+						TemplateID: templates[0].ID,
+					})
+					require.NoError(t, err)
+					require.Len(t, versions, 5)
+				},
+			},
+			// Resource deleted
 		},
 	})
 }