diff --git a/AGENTS.md b/AGENTS.md index 63ba37b..f37d146 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -89,6 +89,7 @@ The Go file, the `TypeName` (`req.ProviderTypeName + "_"`), the `examples/ - **Detect "is this a create?" with `req.State.Raw.IsNull()`.** Import populates state without ever running `Create()`, so this is true only for a genuine create and doesn't misfire on the first plan after `terraform import`. Pair it with `req.ConfigValue.IsNull()` — the attribute is *definitely* omitted, not merely unknown — to enforce "required on create" at plan time. (#383) - **Check a collection's null/unknown-ness explicitly; never rely on `len()` alone.** `len(data.Xs) == 0` conflates null ("user omitted it"), unknown ("not decided yet"), and empty ("user wrote `[]`") — three cases that usually need different handling at plan time. Test `IsNull()`/`IsUnknown()` on the framework value first, and only reason about length once the value is known. (#383) - **Many stock validators skip null/unknown values.** e.g. `listvalidator.SizeAtLeast(1)` early-returns on both — which is what makes it compatible with `Optional` — so it rejects an explicit `[]` but can't reject an *omitted* attribute. If null itself must be rejected, handle it explicitly in a plan modifier or custom validator. (#383) +- **State movers run before `Configure()` — they must be fully offline.** Terraform Core calls the `MoveResourceState` RPC before `ConfigureProvider` (hashicorp/terraform#35922), so inside a `StateMover` `r.data` is always nil — any dependence on the client or `DefaultOrganizationID` fails deterministically for every `moved`-block user. Carry over only what the source state contains, write null for values the mover can't know, and let the first apply adopt the configured value (`resolveOrganizationID`, adoption-friendly `RequiresReplaceIf`, warn-and-preserve `Read`). (coder/dogfood#453) ## Testing patterns @@ -97,6 +98,7 @@ The Go file, the `TypeName` (`req.ProviderTypeName + "_"`), the `examples/ - **Tests that reach plan/apply need a reachable server.** `Configure()` calls `client.User(ctx, Me)` (to resolve the default org) and `client.Entitlements(ctx)`, so a bogus URL fails with connection-refused even for `PlanOnly`. Use `newMockServer(nil)` (from `provider_headers_test.go`) for plan-only/deferral unit tests. - **Deferral tests:** inject unknown values with a `terraform_data.x.output` reference, then assert the plan succeeds using `PlanOnly: true` + `ExpectNonEmptyPlan: true` (PlanOnly with a non-empty plan otherwise errors with "The non-refresh plan was not empty"). - **Reproduce the "unknown var" class of bug** with required (no-default) variables via `TestStep.ConfigVariables`: the validate walk evaluates required vars as unknown, which is exactly where the `#305` family of bugs surfaced. Literal-interpolated configs and vars-with-defaults do *not* catch it. +- **Test `moved` blocks through the real lifecycle, not by invoking the mover directly.** A hand-built `StateMover` request with pre-populated `r.data` proves nothing about RPC ordering — exactly how coder/dogfood#453 slipped through. Use a two-step `resource.Test`: step 1 persists old-schema state via the in-test `legacyCoderdProvider`, step 2 runs the real factories with the `moved` block and asserts plan actions and final state. If the mover checks the source provider address, set `t.Setenv(resource.EnvTfAccProviderNamespace, "coder")` (the harness defaults to `hashicorp/`; precludes `t.Parallel()`). - **Acceptance tests** (the server-backed `TestAcc*` ones) share one Coder instance and therefore **cannot run subtests in parallel** — hence golangci's `paralleltest.ignore-missing-subtests: true`. Use `statecheck`/`ConfigPlanChecks` to assert plan/state. ## Boundaries diff --git a/internal/provider/agents_default_model_resource.go b/internal/provider/agents_default_model_resource.go index 8ad538c..c34e79a 100644 --- a/internal/provider/agents_default_model_resource.go +++ b/internal/provider/agents_default_model_resource.go @@ -56,6 +56,14 @@ type legacyDefaultAgentsModelResourceModel struct { ModelID types.String `tfsdk:"model_id"` } +func legacyDefaultAgentsModelStateDetail(modelID uuid.UUID) string { + return fmt.Sprintf( + "State moved from coderd_default_agents_model for model %s predates organization scoping and does not record its organization. "+ + "Run `terraform apply` to adopt the configured `organization_id` in place.", + modelID, + ) +} + func (r *AgentsDefaultModelResource) Metadata(ctx context.Context, req resource.MetadataRequest, resp *resource.MetadataResponse) { resp.TypeName = req.ProviderTypeName + "_agents_default_model" } @@ -91,7 +99,11 @@ func (r *AgentsDefaultModelResource) Schema(ctx context.Context, req resource.Sc CustomType: UUIDType, Required: true, PlanModifiers: []planmodifier.String{ - stringplanmodifier.RequiresReplace(), + stringplanmodifier.RequiresReplaceIf( + agentsModelOrganizationRequiresReplace, + agentsModelOrganizationRequiresReplaceDescription, + agentsModelOrganizationRequiresReplaceDescription, + ), }, }, "model_id": schema.StringAttribute{ @@ -122,14 +134,6 @@ func (r *AgentsDefaultModelResource) MoveState(ctx context.Context) []resource.S !strings.HasSuffix(req.SourceProviderAddress, "coder/coderd") { return } - if r.data == nil { - resp.Diagnostics.AddError( - "Unable to Move Default Agents Model State", - "The provider was not configured before Terraform attempted to move coderd_default_agents_model state.", - ) - return - } - if req.SourceState == nil { resp.Diagnostics.AddError( "Unable to Move Default Agents Model State", @@ -153,10 +157,12 @@ func (r *AgentsDefaultModelResource) MoveState(ctx context.Context) []resource.S return } - organizationID := r.data.DefaultOrganizationID + // MoveResourceState runs before provider configuration, so the + // default organization cannot be resolved here. Leave it null and + // let the subsequent apply adopt the configured organization_id. resp.Diagnostics.Append(resp.TargetState.Set(ctx, AgentsDefaultModelResourceModel{ - ID: UUIDValue(organizationID), - OrganizationID: UUIDValue(organizationID), + ID: NewUUIDNull(), + OrganizationID: NewUUIDNull(), ModelID: UUIDValue(modelID), })...) }, @@ -204,6 +210,16 @@ func (r *AgentsDefaultModelResource) Read(ctx context.Context, req resource.Read return } + if state.OrganizationID.IsNull() || state.OrganizationID.IsUnknown() { + // State moved from coderd_default_agents_model lacks an organization. + // Read cannot see config, so preserve it until apply adopts the + // configured organization_id. + resp.Diagnostics.AddWarning( + "Legacy Default Agents Model State", + legacyDefaultAgentsModelStateDetail(state.ModelID.ValueUUID()), + ) + return + } organizationID := state.OrganizationID.ValueUUID() configs, err := r.experimentalClient().ChatModels(ctx, organizationID) if err != nil { @@ -239,11 +255,14 @@ func (r *AgentsDefaultModelResource) Update(ctx context.Context, req resource.Up return } + organizationID := resolveOrganizationID(state.OrganizationID, plan.OrganizationID, plan.ModelID.ValueUUID(), &resp.Diagnostics) + if resp.Diagnostics.HasError() { + return + } tflog.Info(ctx, "updating default Agents model", map[string]any{ - "organization_id": state.OrganizationID.ValueString(), + "organization_id": organizationID.String(), "model_id": plan.ModelID.ValueString(), }) - organizationID := state.OrganizationID.ValueUUID() updated, err := r.setDefault(ctx, organizationID, plan.ModelID.ValueUUID()) if err != nil { resp.Diagnostics.Append(r.agentsDefaultModelDiag(ctx, "update", organizationID, plan.ModelID.ValueUUID(), err)...) diff --git a/internal/provider/agents_default_model_resource_test.go b/internal/provider/agents_default_model_resource_test.go index fe4cf02..9c372fc 100644 --- a/internal/provider/agents_default_model_resource_test.go +++ b/internal/provider/agents_default_model_resource_test.go @@ -2,6 +2,7 @@ package provider import ( "context" + "encoding/json" "errors" "fmt" "net/http" @@ -10,19 +11,27 @@ import ( "os" "sync/atomic" "testing" + "time" "github.com/coder/coder/v2/codersdk" "github.com/coder/terraform-provider-coderd/integration" "github.com/google/uuid" + "github.com/hashicorp/terraform-plugin-framework/providerserver" fwresource "github.com/hashicorp/terraform-plugin-framework/resource" "github.com/hashicorp/terraform-plugin-framework/resource/schema" "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" "github.com/hashicorp/terraform-plugin-framework/tfsdk" "github.com/hashicorp/terraform-plugin-framework/types" + "github.com/hashicorp/terraform-plugin-go/tfprotov6" "github.com/hashicorp/terraform-plugin-go/tftypes" "github.com/hashicorp/terraform-plugin-testing/config" "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/knownvalue" + "github.com/hashicorp/terraform-plugin-testing/plancheck" + "github.com/hashicorp/terraform-plugin-testing/statecheck" "github.com/hashicorp/terraform-plugin-testing/terraform" + "github.com/hashicorp/terraform-plugin-testing/tfjsonpath" + "github.com/hashicorp/terraform-plugin-testing/tfversion" "github.com/stretchr/testify/require" ) @@ -48,9 +57,8 @@ func TestAgentsDefaultModelMoveState(t *testing.T) { t.Parallel() ctx := t.Context() - organizationID := uuid.New() modelID := uuid.New() - r := &AgentsDefaultModelResource{data: &CoderdProviderData{DefaultOrganizationID: organizationID}} + r := &AgentsDefaultModelResource{} movers := r.MoveState(ctx) require.Len(t, movers, 1) require.NotNil(t, movers[0].SourceSchema) @@ -85,9 +93,56 @@ func TestAgentsDefaultModelMoveState(t *testing.T) { var got AgentsDefaultModelResourceModel require.False(t, resp.TargetState.Get(ctx, &got).HasError()) - require.Equal(t, organizationID, got.ID.ValueUUID()) - require.Equal(t, organizationID, got.OrganizationID.ValueUUID()) + require.True(t, got.ID.IsNull()) + require.True(t, got.OrganizationID.IsNull()) require.Equal(t, modelID, got.ModelID.ValueUUID()) + + for _, tc := range []struct { + name string + req fwresource.MoveStateRequest + }{ + { + name: "wrong source type", + req: fwresource.MoveStateRequest{ + SourceProviderAddress: "registry.example.com/coder/coderd", + SourceSchemaVersion: 0, + SourceState: &sourceState, + SourceTypeName: "coderd_other", + }, + }, + { + name: "wrong schema version", + req: fwresource.MoveStateRequest{ + SourceProviderAddress: "registry.example.com/coder/coderd", + SourceSchemaVersion: 1, + SourceState: &sourceState, + SourceTypeName: "coderd_default_agents_model", + }, + }, + { + name: "wrong provider address", + req: fwresource.MoveStateRequest{ + SourceProviderAddress: "registry.example.com/hashicorp/coderd", + SourceSchemaVersion: 0, + SourceState: &sourceState, + SourceTypeName: "coderd_default_agents_model", + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + resp := &fwresource.MoveStateResponse{ + TargetState: tfsdk.State{ + Schema: targetSchema, + Raw: tftypes.NewValue(targetSchema.Type().TerraformType(ctx), nil), + }, + } + movers[0].StateMover(ctx, tc.req, resp) + require.False(t, resp.Diagnostics.HasError(), resp.Diagnostics) + require.True(t, resp.TargetState.Raw.IsNull()) + }) + } } func TestAgentsDefaultModelIDPlanModifier(t *testing.T) { @@ -437,6 +492,169 @@ resource "coderd_agents_default_model" "default" { }) } +// TestAgentsDefaultModelMovedBlockMigration reproduces the coder/dogfood +// v0.0.23 -> v0.0.24 upgrade (coder/dogfood#453): step 1 writes real +// coderd_default_agents_model state with the legacy schema, step 2 runs the +// real provider with a `moved` block to coderd_agents_default_model, an +// adopted coderd_agents_model, and organization_id sourced from +// data.coderd_organization (is_default = true). Terraform core invokes the +// MoveResourceState RPC before ConfigureProvider (hashicorp/terraform#35922), +// so the mover must work without provider data; before the fix this plan +// failed with "The provider was not configured before Terraform attempted to +// move coderd_default_agents_model state.". +func TestAgentsDefaultModelMovedBlockMigration(t *testing.T) { + // The state mover only accepts source addresses from coder/coderd, and the + // test framework defaults to the hashicorp namespace. t.Setenv also + // prevents t.Parallel(). + t.Setenv(resource.EnvTfAccProviderNamespace, "coder") + + orgID := uuid.New() + ts := time.Unix(1700000000, 0).UTC() + model := codersdk.ChatModel{ + ID: uuid.New(), + OrganizationID: orgID, + AIProviderID: uuid.New(), + Model: "claude-3-5-sonnet-20241022", + DisplayName: "Claude Sonnet", + Enabled: true, + ContextLimit: 200000, + CompressionThreshold: 70, + CreatedAt: ts, + UpdatedAt: ts, + } + + var modelPatched, defaultPatched atomic.Bool + srv := fakeAgentsDefaultModelMigrationServer(t, orgID, model, &modelPatched, &defaultPatched) + + providerBlock := `provider "coderd" { + url = "` + srv.URL + `" + token = "test-token" +} +` + modelArgs := ` ai_provider_id = "` + model.AIProviderID.String() + `" + model = "` + model.Model + `" + context_limit = 200000 +} +` + legacyFactories := map[string]func() (tfprotov6.ProviderServer, error){ + "coderd": providerserver.NewProtocol6WithError(&legacyCoderdProvider{model: model}), + } + + resource.Test(t, resource.TestCase{ + IsUnitTest: true, + // Cross-resource-type moved blocks (the MoveResourceState RPC) require + // Terraform 1.8+; older versions reject the moved block with "Resource + // type mismatch". + TerraformVersionChecks: []tfversion.TerraformVersionCheck{ + tfversion.SkipBelow(tfversion.Version1_8_0), + }, + Steps: []resource.TestStep{ + { + ProtoV6ProviderFactories: legacyFactories, + Config: providerBlock + ` +resource "coderd_agents_model" "test" { +` + modelArgs + ` +resource "coderd_default_agents_model" "this" { + model_id = coderd_agents_model.test.id +} +`, + }, + { + ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, + Config: providerBlock + ` +data "coderd_organization" "default" { + is_default = true +} + +resource "coderd_agents_model" "test" { + organization_id = data.coderd_organization.default.id +` + modelArgs + ` +moved { + from = coderd_default_agents_model.this + to = coderd_agents_default_model.this +} + +resource "coderd_agents_default_model" "this" { + organization_id = data.coderd_organization.default.id + model_id = coderd_agents_model.test.id +} +`, + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("coderd_agents_model.test", plancheck.ResourceActionUpdate), + plancheck.ExpectResourceAction("coderd_agents_default_model.this", plancheck.ResourceActionUpdate), + }, + }, + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue("coderd_agents_default_model.this", tfjsonpath.New("id"), knownvalue.StringExact(orgID.String())), + statecheck.ExpectKnownValue("coderd_agents_default_model.this", tfjsonpath.New("organization_id"), knownvalue.StringExact(orgID.String())), + statecheck.ExpectKnownValue("coderd_agents_default_model.this", tfjsonpath.New("model_id"), knownvalue.StringExact(model.ID.String())), + statecheck.ExpectKnownValue("coderd_agents_model.test", tfjsonpath.New("organization_id"), knownvalue.StringExact(orgID.String())), + }, + }, + }, + }) + + require.True(t, modelPatched.Load(), "expected the model adoption apply to PATCH the organization-scoped route") + require.True(t, defaultPatched.Load(), "expected the moved default model apply to PATCH is_default on the organization-scoped route") +} + +// fakeAgentsDefaultModelMigrationServer serves everything the moved-block +// migration exercises: provider Configure, the coderd_organization data +// source, and the organization-scoped chat model routes. PATCHes carrying +// is_default are recorded separately from model updates because both +// resources share the same route. +func fakeAgentsDefaultModelMigrationServer(t *testing.T, orgID uuid.UUID, model codersdk.ChatModel, modelPatched, defaultPatched *atomic.Bool) *httptest.Server { + t.Helper() + + defaultModel := model + defaultModel.IsDefault = true + modelPath := fmt.Sprintf("/api/v2/organizations/%s/chats/models/%s", orgID, model.ID) + mux := http.NewServeMux() + mux.HandleFunc("/", func(w http.ResponseWriter, _ *http.Request) { + // Configure fetches the current user and entitlements; the user + // payload decodes acceptably for both. + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprintf(w, `{"id":"%s","username":"admin","organization_ids":["%s"]}`, uuid.NewString(), orgID) + }) + mux.HandleFunc("GET /api/v2/organizations/default", func(w http.ResponseWriter, _ *http.Request) { + writeJSON(w, http.StatusOK, codersdk.Organization{ + MinimalOrganization: codersdk.MinimalOrganization{ID: orgID, Name: "default"}, + IsDefault: true, + }) + }) + mux.HandleFunc(fmt.Sprintf("GET /api/v2/organizations/%s/members/", orgID), func(w http.ResponseWriter, _ *http.Request) { + writeJSON(w, http.StatusOK, []codersdk.OrganizationMemberWithUserData{}) + }) + mux.HandleFunc("GET /api/v2/ai/providers/"+model.AIProviderID.String(), func(w http.ResponseWriter, _ *http.Request) { + writeJSON(w, http.StatusOK, codersdk.AIProvider{ID: model.AIProviderID, Type: "anthropic"}) + }) + mux.HandleFunc("GET "+modelPath, func(w http.ResponseWriter, _ *http.Request) { + writeJSON(w, http.StatusOK, model) + }) + mux.HandleFunc("PATCH "+modelPath, func(w http.ResponseWriter, r *http.Request) { + var req codersdk.UpdateChatModelRequest + require.NoError(t, json.NewDecoder(r.Body).Decode(&req)) + if req.IsDefault != nil && *req.IsDefault { + defaultPatched.Store(true) + writeJSON(w, http.StatusOK, defaultModel) + return + } + modelPatched.Store(true) + writeJSON(w, http.StatusOK, model) + }) + mux.HandleFunc("DELETE "+modelPath, func(w http.ResponseWriter, _ *http.Request) { + // Post-test destroy cleanup. + w.WriteHeader(http.StatusNoContent) + }) + mux.HandleFunc(fmt.Sprintf("GET /api/v2/organizations/%s/chats/models", orgID), func(w http.ResponseWriter, _ *http.Request) { + writeJSON(w, http.StatusOK, codersdk.OrganizationChatModelsResponse{Models: []codersdk.ChatModel{defaultModel}}) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + return srv +} + func TestAccAgentsDefaultModelResource(t *testing.T) { t.Parallel() if os.Getenv("TF_ACC") == "" { diff --git a/internal/provider/agents_model_resource_test.go b/internal/provider/agents_model_resource_test.go index 087ea93..d372273 100644 --- a/internal/provider/agents_model_resource_test.go +++ b/internal/provider/agents_model_resource_test.go @@ -1190,6 +1190,7 @@ func (*legacyCoderdProvider) DataSources(context.Context) []func() datasource.Da func (p *legacyCoderdProvider) Resources(context.Context) []func() frameworkresource.Resource { return []func() frameworkresource.Resource{ func() frameworkresource.Resource { return &legacyAgentsModelResource{model: p.model} }, + func() frameworkresource.Resource { return &legacyDefaultAgentsModelResource{} }, } } @@ -1262,6 +1263,49 @@ func (*legacyAgentsModelResource) Update(context.Context, frameworkresource.Upda func (*legacyAgentsModelResource) Delete(context.Context, frameworkresource.DeleteRequest, *frameworkresource.DeleteResponse) { } +// legacyDefaultAgentsModelResource reproduces the v0.0.23 +// coderd_default_agents_model schema and writes its state without a server. +type legacyDefaultAgentsModelResource struct{} + +var _ frameworkresource.Resource = (*legacyDefaultAgentsModelResource)(nil) + +type legacyDefaultAgentsModelModel struct { + ID types.String `tfsdk:"id"` + ModelID types.String `tfsdk:"model_id"` +} + +func (*legacyDefaultAgentsModelResource) Metadata(_ context.Context, req frameworkresource.MetadataRequest, resp *frameworkresource.MetadataResponse) { + resp.TypeName = req.ProviderTypeName + "_default_agents_model" +} + +func (*legacyDefaultAgentsModelResource) Schema(_ context.Context, _ frameworkresource.SchemaRequest, resp *frameworkresource.SchemaResponse) { + resp.Schema = resourceschema.Schema{ + Attributes: map[string]resourceschema.Attribute{ + "id": resourceschema.StringAttribute{Computed: true}, + "model_id": resourceschema.StringAttribute{Required: true}, + }, + } +} + +func (*legacyDefaultAgentsModelResource) Create(ctx context.Context, req frameworkresource.CreateRequest, resp *frameworkresource.CreateResponse) { + var plan legacyDefaultAgentsModelModel + resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...) + if resp.Diagnostics.HasError() { + return + } + plan.ID = types.StringValue("default") + resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...) +} + +func (*legacyDefaultAgentsModelResource) Read(context.Context, frameworkresource.ReadRequest, *frameworkresource.ReadResponse) { +} + +func (*legacyDefaultAgentsModelResource) Update(context.Context, frameworkresource.UpdateRequest, *frameworkresource.UpdateResponse) { +} + +func (*legacyDefaultAgentsModelResource) Delete(context.Context, frameworkresource.DeleteRequest, *frameworkresource.DeleteResponse) { +} + // fakeChatModelServer serves the provider Configure endpoints plus the // organization-scoped chat model routes the real provider uses after upgrade. func fakeChatModelServer(t *testing.T, orgID uuid.UUID, model codersdk.ChatModel, patched *atomic.Bool) *httptest.Server {