Skip to content

Commit 3fa24e5

Browse files
gersonsosagersonsosa
authored
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
1 parent 454e955 commit 3fa24e5

File tree

4 files changed

+189
-4
lines changed

4 files changed

+189
-4
lines changed

‎ec/acc/deployment_basic_defaults_test.go‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,6 @@ import (
3232
// * Resource declaration in the <kind> {} format. ("integrations_server {}").
3333
// * Topology field overrides over field defaults.
3434
func TestAccDeployment_basic_defaults_first(t *testing.T) {
35-
t.Skip("skip until integrations_server component change is correctly detected https://elasticco.atlassian.net/browse/CP-9334")
36-
3735
resName := "ec_deployment.defaults"
3836
randomName := prefix + acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)
3937
startCfg := "testdata/deployment_basic_defaults_1.tf"

‎ec/acc/deployment_cpu_optimized_test.go‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,6 @@ import (
2525
)
2626

2727
func TestAccDeployment_cpuOptimized(t *testing.T) {
28-
t.Skip("skip until apm component change is correctly detected https://elasticco.atlassian.net/browse/CP-9334")
29-
3028
resName := "ec_deployment.cpu_optimized"
3129
randomName := prefix + acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)
3230
startCfg := "testdata/deployment_cpu_optimized_1.tf"

‎ec/internal/planmodifiers/use_state_unless_template_changed.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import (
3131
// 1. The attribute is not nullable (`isNullable = false`) and the topology's state is nil
3232
// 2. The deployment template attribute has changed
3333
// 3. `migrate_to_latest_hardware` is set to `true` and there is a migration available to be performed
34+
// 4. The state of the parent attribute is nil
3435
func UseStateForUnknownUnlessMigrationIsRequired(resourceKind string, isNullable bool) useStateForUnknownUnlessMigrationIsRequired {
3536
return useStateForUnknownUnlessMigrationIsRequired{resourceKind: resourceKind, isNullable: isNullable}
3637
}
@@ -63,6 +64,17 @@ func (m useStateForUnknownUnlessMigrationIsRequired) PlanModifyInt64(ctx context
6364
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) {
6465
var diags diag.Diagnostics
6566

67+
var parentResState attr.Value
68+
if d := state.GetAttribute(ctx, path.Root(m.resourceKind), &parentResState); d.HasError() {
69+
return false, d
70+
}
71+
72+
resourceIsBeingCreated := parentResState.IsNull()
73+
74+
if resourceIsBeingCreated {
75+
return false, nil
76+
}
77+
6678
if stateValue.IsNull() && !m.isNullable {
6779
return false, nil
6880
}

‎ec/internal/planmodifiers/use_state_unless_template_changed_test.go‎

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,177 @@
1+
// Licensed to Elasticsearch B.V. under one or more contributor
2+
// license agreements. See the NOTICE file distributed with
3+
// this work for additional information regarding copyright
4+
// ownership. Elasticsearch B.V. licenses this file to you under
5+
// the Apache License, Version 2.0 (the "License"); you may
6+
// not use this file except in compliance with the License.
7+
// You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
18+
package planmodifiers_test
19+
20+
import (
21+
"context"
22+
"testing"
23+
24+
"github.com/elastic/cloud-sdk-go/pkg/util/ec"
25+
apmv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/apm/v2"
26+
v2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/deployment/v2"
27+
integrationsserverv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/integrationsserver/v2"
28+
"github.com/elastic/terraform-provider-ec/ec/internal/planmodifiers"
29+
"github.com/elastic/terraform-provider-ec/ec/internal/util"
30+
"github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
31+
"github.com/hashicorp/terraform-plugin-framework/tfsdk"
32+
"github.com/hashicorp/terraform-plugin-framework/types"
33+
"github.com/stretchr/testify/require"
34+
)
35+
36+
func TestUseStateForUnknownUnlessMigrationIsRequired_Apm_PlanModifyInt64(t *testing.T) {
37+
ver := 1
38+
tests := []struct {
39+
name string
40+
resourceKind string
41+
nullable bool
42+
state *v2.Deployment
43+
plan v2.Deployment
44+
planValue types.Int64
45+
stateValue types.Int64
46+
configValue types.Int64
47+
expectedPlanValue types.Int64
48+
}{
49+
{
50+
name: "should update the plan value to null if apm has state null",
51+
resourceKind: "apm",
52+
nullable: true,
53+
state: &v2.Deployment{
54+
Apm: &apmv2.Apm{
55+
InstanceConfigurationVersion: nil,
56+
},
57+
},
58+
plan: v2.Deployment{
59+
Apm: &apmv2.Apm{},
60+
},
61+
expectedPlanValue: types.Int64Null(),
62+
planValue: types.Int64Unknown(),
63+
stateValue: types.Int64Null(),
64+
configValue: types.Int64Null(),
65+
},
66+
{
67+
name: "should update the plan value if apm has state value",
68+
resourceKind: "apm",
69+
nullable: true,
70+
state: &v2.Deployment{
71+
Apm: &apmv2.Apm{
72+
InstanceConfigurationVersion: &ver,
73+
},
74+
},
75+
plan: v2.Deployment{
76+
Apm: &apmv2.Apm{},
77+
},
78+
expectedPlanValue: types.Int64Value(1),
79+
planValue: types.Int64Unknown(),
80+
stateValue: types.Int64Value(1),
81+
configValue: types.Int64Null(),
82+
},
83+
{
84+
name: "should keep apm instance_configuration_version unknown when the resource is being created",
85+
resourceKind: "apm",
86+
nullable: true,
87+
state: &v2.Deployment{},
88+
plan: v2.Deployment{
89+
Apm: &apmv2.Apm{
90+
Size: ec.String("2g"),
91+
},
92+
},
93+
expectedPlanValue: types.Int64Unknown(),
94+
planValue: types.Int64Unknown(),
95+
stateValue: types.Int64Null(),
96+
configValue: types.Int64Null(),
97+
},
98+
{
99+
name: "should update the plan value to null if apm has state null",
100+
resourceKind: "integrations_server",
101+
nullable: true,
102+
state: &v2.Deployment{
103+
IntegrationsServer: &integrationsserverv2.IntegrationsServer{
104+
InstanceConfigurationVersion: nil,
105+
},
106+
},
107+
plan: v2.Deployment{
108+
IntegrationsServer: &integrationsserverv2.IntegrationsServer{},
109+
},
110+
expectedPlanValue: types.Int64Null(),
111+
planValue: types.Int64Unknown(),
112+
stateValue: types.Int64Null(),
113+
configValue: types.Int64Null(),
114+
},
115+
{
116+
name: "should update the plan value if apm has state value",
117+
resourceKind: "integrations_server",
118+
nullable: true,
119+
state: &v2.Deployment{
120+
IntegrationsServer: &integrationsserverv2.IntegrationsServer{
121+
InstanceConfigurationVersion: &ver,
122+
},
123+
},
124+
plan: v2.Deployment{
125+
IntegrationsServer: &integrationsserverv2.IntegrationsServer{},
126+
},
127+
expectedPlanValue: types.Int64Value(1),
128+
planValue: types.Int64Unknown(),
129+
stateValue: types.Int64Value(1),
130+
configValue: types.Int64Null(),
131+
},
132+
{
133+
name: "should keep apm instance_configuration_version unknown when the resource is being created",
134+
resourceKind: "integrations_server",
135+
nullable: true,
136+
state: &v2.Deployment{},
137+
plan: v2.Deployment{
138+
IntegrationsServer: &integrationsserverv2.IntegrationsServer{
139+
Size: ec.String("2g"),
140+
},
141+
},
142+
expectedPlanValue: types.Int64Unknown(),
143+
planValue: types.Int64Unknown(),
144+
stateValue: types.Int64Null(),
145+
configValue: types.Int64Null(),
146+
},
147+
}
148+
149+
for _, tt := range tests {
150+
t.Run(tt.name, func(t *testing.T) {
151+
stateRaw := util.TfTypesValueFromGoTypeValue(t, tt.state, v2.DeploymentSchema().Type())
152+
planRaw := util.TfTypesValueFromGoTypeValue(t, tt.plan, v2.DeploymentSchema().Type())
153+
req := planmodifier.Int64Request{
154+
PlanValue: tt.planValue,
155+
StateValue: tt.stateValue,
156+
ConfigValue: tt.configValue,
157+
State: tfsdk.State{
158+
Raw: stateRaw,
159+
Schema: v2.DeploymentSchema(),
160+
},
161+
Plan: tfsdk.Plan{
162+
Raw: planRaw,
163+
Schema: v2.DeploymentSchema(),
164+
},
165+
}
166+
167+
resp := planmodifier.Int64Response{
168+
PlanValue: tt.planValue,
169+
}
170+
modifier := planmodifiers.UseStateForUnknownUnlessMigrationIsRequired(tt.resourceKind, tt.nullable)
171+
172+
modifier.PlanModifyInt64(context.Background(), req, &resp)
173+
174+
require.Equal(t, tt.expectedPlanValue, resp.PlanValue)
175+
})
176+
}
177+
}

0 commit comments

Comments
 (0)