From 3fa24e5b830ed6061982a32dfa0b1e6a7fe09d0b Mon Sep 17 00:00:00 2001 From: Gerson Sosa <4366859+gersonsosa@users.noreply.github.com> Date: Thu, 28 Nov 2024 19:24:53 +0100 Subject: [PATCH] bugfix: don't use nil state for a computed attribute (#878) * bugfix: don't use nil state for a computed attribute If a computed attribute parent state is null, don't try to use the state to avoid inconsistency errors when the apply process finishes the state will still be used in the previous cases to preserve the same functionality tests: remove skip from acc tests with stateless resources add unit tests to use_state_unless_template_changed_test --- ec/acc/deployment_basic_defaults_test.go | 2 - ec/acc/deployment_cpu_optimized_test.go | 2 - .../use_state_unless_template_changed.go | 12 ++ .../use_state_unless_template_changed_test.go | 177 ++++++++++++++++++ 4 files changed, 189 insertions(+), 4 deletions(-) create mode 100644 ec/internal/planmodifiers/use_state_unless_template_changed_test.go diff --git a/ec/acc/deployment_basic_defaults_test.go b/ec/acc/deployment_basic_defaults_test.go index 5a780e8ed..edcb0eeaf 100644 --- a/ec/acc/deployment_basic_defaults_test.go +++ b/ec/acc/deployment_basic_defaults_test.go @@ -32,8 +32,6 @@ import ( // * Resource declaration in the {} format. ("integrations_server {}"). // * Topology field overrides over field defaults. func TestAccDeployment_basic_defaults_first(t *testing.T) { - t.Skip("skip until integrations_server component change is correctly detected https://elasticco.atlassian.net/browse/CP-9334") - resName := "ec_deployment.defaults" randomName := prefix + acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum) startCfg := "testdata/deployment_basic_defaults_1.tf" diff --git a/ec/acc/deployment_cpu_optimized_test.go b/ec/acc/deployment_cpu_optimized_test.go index acc663a91..ec0769c70 100644 --- a/ec/acc/deployment_cpu_optimized_test.go +++ b/ec/acc/deployment_cpu_optimized_test.go @@ -25,8 +25,6 @@ import ( ) func TestAccDeployment_cpuOptimized(t *testing.T) { - t.Skip("skip until apm component change is correctly detected https://elasticco.atlassian.net/browse/CP-9334") - resName := "ec_deployment.cpu_optimized" randomName := prefix + acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum) startCfg := "testdata/deployment_cpu_optimized_1.tf" diff --git a/ec/internal/planmodifiers/use_state_unless_template_changed.go b/ec/internal/planmodifiers/use_state_unless_template_changed.go index 89ce051a5..da96e52d2 100644 --- a/ec/internal/planmodifiers/use_state_unless_template_changed.go +++ b/ec/internal/planmodifiers/use_state_unless_template_changed.go @@ -31,6 +31,7 @@ import ( // 1. The attribute is not nullable (`isNullable = false`) and the topology's state is nil // 2. The deployment template attribute has changed // 3. `migrate_to_latest_hardware` is set to `true` and there is a migration available to be performed +// 4. The state of the parent attribute is nil func UseStateForUnknownUnlessMigrationIsRequired(resourceKind string, isNullable bool) useStateForUnknownUnlessMigrationIsRequired { return useStateForUnknownUnlessMigrationIsRequired{resourceKind: resourceKind, isNullable: isNullable} } @@ -63,6 +64,17 @@ func (m useStateForUnknownUnlessMigrationIsRequired) PlanModifyInt64(ctx context func (m useStateForUnknownUnlessMigrationIsRequired) UseState(ctx context.Context, configValue attr.Value, plan tfsdk.Plan, state tfsdk.State, planValue attr.Value, stateValue attr.Value) (bool, diag.Diagnostics) { var diags diag.Diagnostics + var parentResState attr.Value + if d := state.GetAttribute(ctx, path.Root(m.resourceKind), &parentResState); d.HasError() { + return false, d + } + + resourceIsBeingCreated := parentResState.IsNull() + + if resourceIsBeingCreated { + return false, nil + } + if stateValue.IsNull() && !m.isNullable { return false, nil } diff --git a/ec/internal/planmodifiers/use_state_unless_template_changed_test.go b/ec/internal/planmodifiers/use_state_unless_template_changed_test.go new file mode 100644 index 000000000..e019c812c --- /dev/null +++ b/ec/internal/planmodifiers/use_state_unless_template_changed_test.go @@ -0,0 +1,177 @@ +// Licensed to Elasticsearch B.V. under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Elasticsearch B.V. licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package planmodifiers_test + +import ( + "context" + "testing" + + "github.com/elastic/cloud-sdk-go/pkg/util/ec" + apmv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/apm/v2" + v2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/deployment/v2" + integrationsserverv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/integrationsserver/v2" + "github.com/elastic/terraform-provider-ec/ec/internal/planmodifiers" + "github.com/elastic/terraform-provider-ec/ec/internal/util" + "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/stretchr/testify/require" +) + +func TestUseStateForUnknownUnlessMigrationIsRequired_Apm_PlanModifyInt64(t *testing.T) { + ver := 1 + tests := []struct { + name string + resourceKind string + nullable bool + state *v2.Deployment + plan v2.Deployment + planValue types.Int64 + stateValue types.Int64 + configValue types.Int64 + expectedPlanValue types.Int64 + }{ + { + name: "should update the plan value to null if apm has state null", + resourceKind: "apm", + nullable: true, + state: &v2.Deployment{ + Apm: &apmv2.Apm{ + InstanceConfigurationVersion: nil, + }, + }, + plan: v2.Deployment{ + Apm: &apmv2.Apm{}, + }, + expectedPlanValue: types.Int64Null(), + planValue: types.Int64Unknown(), + stateValue: types.Int64Null(), + configValue: types.Int64Null(), + }, + { + name: "should update the plan value if apm has state value", + resourceKind: "apm", + nullable: true, + state: &v2.Deployment{ + Apm: &apmv2.Apm{ + InstanceConfigurationVersion: &ver, + }, + }, + plan: v2.Deployment{ + Apm: &apmv2.Apm{}, + }, + expectedPlanValue: types.Int64Value(1), + planValue: types.Int64Unknown(), + stateValue: types.Int64Value(1), + configValue: types.Int64Null(), + }, + { + name: "should keep apm instance_configuration_version unknown when the resource is being created", + resourceKind: "apm", + nullable: true, + state: &v2.Deployment{}, + plan: v2.Deployment{ + Apm: &apmv2.Apm{ + Size: ec.String("2g"), + }, + }, + expectedPlanValue: types.Int64Unknown(), + planValue: types.Int64Unknown(), + stateValue: types.Int64Null(), + configValue: types.Int64Null(), + }, + { + name: "should update the plan value to null if apm has state null", + resourceKind: "integrations_server", + nullable: true, + state: &v2.Deployment{ + IntegrationsServer: &integrationsserverv2.IntegrationsServer{ + InstanceConfigurationVersion: nil, + }, + }, + plan: v2.Deployment{ + IntegrationsServer: &integrationsserverv2.IntegrationsServer{}, + }, + expectedPlanValue: types.Int64Null(), + planValue: types.Int64Unknown(), + stateValue: types.Int64Null(), + configValue: types.Int64Null(), + }, + { + name: "should update the plan value if apm has state value", + resourceKind: "integrations_server", + nullable: true, + state: &v2.Deployment{ + IntegrationsServer: &integrationsserverv2.IntegrationsServer{ + InstanceConfigurationVersion: &ver, + }, + }, + plan: v2.Deployment{ + IntegrationsServer: &integrationsserverv2.IntegrationsServer{}, + }, + expectedPlanValue: types.Int64Value(1), + planValue: types.Int64Unknown(), + stateValue: types.Int64Value(1), + configValue: types.Int64Null(), + }, + { + name: "should keep apm instance_configuration_version unknown when the resource is being created", + resourceKind: "integrations_server", + nullable: true, + state: &v2.Deployment{}, + plan: v2.Deployment{ + IntegrationsServer: &integrationsserverv2.IntegrationsServer{ + Size: ec.String("2g"), + }, + }, + expectedPlanValue: types.Int64Unknown(), + planValue: types.Int64Unknown(), + stateValue: types.Int64Null(), + configValue: types.Int64Null(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + stateRaw := util.TfTypesValueFromGoTypeValue(t, tt.state, v2.DeploymentSchema().Type()) + planRaw := util.TfTypesValueFromGoTypeValue(t, tt.plan, v2.DeploymentSchema().Type()) + req := planmodifier.Int64Request{ + PlanValue: tt.planValue, + StateValue: tt.stateValue, + ConfigValue: tt.configValue, + State: tfsdk.State{ + Raw: stateRaw, + Schema: v2.DeploymentSchema(), + }, + Plan: tfsdk.Plan{ + Raw: planRaw, + Schema: v2.DeploymentSchema(), + }, + } + + resp := planmodifier.Int64Response{ + PlanValue: tt.planValue, + } + modifier := planmodifiers.UseStateForUnknownUnlessMigrationIsRequired(tt.resourceKind, tt.nullable) + + modifier.PlanModifyInt64(context.Background(), req, &resp) + + require.Equal(t, tt.expectedPlanValue, resp.PlanValue) + }) + } +}