chore: minor touch-ups

Signed-off-by: Danny Kopping <dannykopping@gmail.com>
coder · dannykopping · May 14, 2025 · Apr 25, 2025 · Apr 25, 2025 · Apr 25, 2025
commit b29e8fa22f7afa1298c27e77f9c37f10d9bfc960
diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go
@@ -1841,7 +1841,7 @@ func (s *server) notifyWorkspaceDeleted(ctx context.Context, workspace database.
 }
 
 func (s *server) notifyPrebuiltWorkspaceResourceReplacement(ctx context.Context, workspace database.Workspace, build database.WorkspaceBuild, claimantID uuid.UUID, replacements []*sdkproto.ResourceReplacement) {
-	if claimantID == uuid.Nil {
+	if claimantID == uuid.Nil || len(replacements) == 0 {
 		// This is not a prebuild claim.
 		return
 	}
@@ -1878,8 +1878,9 @@ func (s *server) notifyPrebuiltWorkspaceResourceReplacement(ctx context.Context,
 			// Associate this notification with all the related entities.
 			workspace.ID, workspace.OwnerID, workspace.TemplateID, workspace.OrganizationID,
 		); err != nil {
-			s.Logger.Warn(ctx, "failed to notify of prebuilt workspace resource replacement", slog.Error(err))
-			break
+			s.Logger.Warn(ctx, "failed to notify of prebuilt workspace resource replacement", slog.Error(err),
+				slog.F("user_id", templateAdmin.ID.String()), slog.F("user_email", templateAdmin.Email))
+			continue
 		}
 	}
 }

diff --git a/provisioner/terraform/executor.go b/provisioner/terraform/executor.go
@@ -300,10 +300,14 @@ func (e *executor) plan(ctx, killCtx context.Context, env, vars []string, logr l
 	graphTimings := newTimingAggregator(database.ProvisionerJobTimingStageGraph)
 	graphTimings.ingest(createGraphTimingsEvent(timingGraphStart))
 
-	state, plan, replacements, err := e.planResources(ctx, killCtx, planfilePath)
+	state, plan, err := e.planResources(ctx, killCtx, planfilePath)
 	if err != nil {
 		graphTimings.ingest(createGraphTimingsEvent(timingGraphErrored))
-		return nil, err
+		return nil, xerrors.Errorf("plan resources: %w", err)
+	}
+	planJSON, err := json.Marshal(plan)
+	if err != nil {
+		return nil, xerrors.Errorf("marshal plan: %w", err)
 	}
 
 	graphTimings.ingest(createGraphTimingsEvent(timingGraphComplete))
@@ -312,17 +316,22 @@ func (e *executor) plan(ctx, killCtx context.Context, env, vars []string, logr l
 	// the point of prebuilding if the expensive resource is replaced once claimed!
 	var (
 		isPrebuildClaimAttempt = !destroy && metadata.GetPrebuildClaimForUserId() != ""
-		reps                   []*proto.ResourceReplacement
+		resReps                []*proto.ResourceReplacement
 	)
-	if count := len(replacements); count > 0 && isPrebuildClaimAttempt {
-		// TODO(dannyk): we should log drift always (not just during prebuild claim attempts); we're validating that this output
-		//				 will not be overwhelming for end-users, but it'll certainly be super valuable for template admins
-		//				 to diagnose this resource replacement issue, at least.
-		e.logDrift(ctx, killCtx, planfilePath, logr)
-
-		reps = make([]*proto.ResourceReplacement, 0, len(replacements))
-		for n, p := range replacements {
-			reps = append(reps, &proto.ResourceReplacement{
+	if repsFromPlan := findResourceReplacements(plan); len(repsFromPlan) > 0 {
+		if isPrebuildClaimAttempt {
+			// TODO(dannyk): we should log drift always (not just during prebuild claim attempts); we're validating that this output
+			//				 will not be overwhelming for end-users, but it'll certainly be super valuable for template admins
+			//				 to diagnose this resource replacement issue, at least.
+			//				 Once prebuilds moves out of beta, consider deleting this condition.
+
+			// Lock held before calling (see top of method).
+			e.logDrift(ctx, killCtx, planfilePath, logr)
+		}
+
+		resReps = make([]*proto.ResourceReplacement, 0, len(repsFromPlan))
+		for n, p := range repsFromPlan {
+			resReps = append(resReps, &proto.ResourceReplacement{
 				Resource: n,
 				Paths:    p,
 			})
@@ -335,8 +344,8 @@ func (e *executor) plan(ctx, killCtx context.Context, env, vars []string, logr l
 		ExternalAuthProviders: state.ExternalAuthProviders,
 		Timings:               append(e.timings.aggregate(), graphTimings.aggregate()...),
 		Presets:               state.Presets,
-		Plan:                  plan,
-		ResourceReplacements:  reps,
+		Plan:                  planJSON,
+		ResourceReplacements:  resReps,
 	}, nil
 }
 
@@ -358,18 +367,18 @@ func onlyDataResources(sm tfjson.StateModule) tfjson.StateModule {
 }
 
 // planResources must only be called while the lock is held.
-func (e *executor) planResources(ctx, killCtx context.Context, planfilePath string) (*State, json.RawMessage, resourceReplacements, error) {
+func (e *executor) planResources(ctx, killCtx context.Context, planfilePath string) (*State, *tfjson.Plan, error) {
 	ctx, span := e.server.startTrace(ctx, tracing.FuncName())
 	defer span.End()
 
 	plan, err := e.parsePlan(ctx, killCtx, planfilePath)
 	if err != nil {
-		return nil, nil, nil, xerrors.Errorf("show terraform plan file: %w", err)
+		return nil, nil, xerrors.Errorf("show terraform plan file: %w", err)
 	}
 
 	rawGraph, err := e.graph(ctx, killCtx)
 	if err != nil {
-		return nil, nil, nil, xerrors.Errorf("graph: %w", err)
+		return nil, nil, xerrors.Errorf("graph: %w", err)
 	}
 	modules := []*tfjson.StateModule{}
 	if plan.PriorState != nil {
@@ -387,15 +396,10 @@ func (e *executor) planResources(ctx, killCtx context.Context, planfilePath stri
 
 	state, err := ConvertState(ctx, modules, rawGraph, e.server.logger)
 	if err != nil {
-		return nil, nil, nil, err
-	}
-
-	planJSON, err := json.Marshal(plan)
-	if err != nil {
-		return nil, nil, nil, err
+		return nil, nil, err
 	}
 
-	return state, planJSON, findResourceReplacements(plan), nil
+	return state, plan, nil
 }
 
 // parsePlan must only be called while the lock is held.

diff --git a/provisioner/terraform/resource_replacements.go b/provisioner/terraform/resource_replacements.go
@@ -42,7 +42,18 @@ func findResourceReplacements(plan *tfjson.Plan) resourceReplacements {
 			continue
 		}
 
-		// Replacing our resources, no problem!
+		// Replacing our resources: could be a problem - but we ignore since they're "virtual" resources. If any of these
+		// resources' attributes are referenced by non-coder resources, those will show up as transitive changes there.
+		// i.e. if the coder_agent.id attribute is used in docker_container.env
+		//
+		// Replacing our resources is not strictly a problem in and of itself.
+		//
+		// NOTE:
+		// We may need to special-case coder_agent in the future. Currently, coder_agent is replaced on every build
+		// because it only supports Create but not Update: https://github.com/coder/terraform-provider-coder/blob/5648efb/provider/agent.go#L28
+		// When we can modify an agent's attributes, some of which may be immutable (like "arch") and some may not (like "env"),
+		// then we'll have to handle this specifically.
+		// This will only become relevant once we support multiple agents: https://github.com/coder/coder/issues/17388
 		if strings.Index(ch.Type, "coder_") == 0 {
 			continue
 		}