From 355d8ea3f76475b24d0f0bbce02fa6729c4ee852 Mon Sep 17 00:00:00 2001 From: Toby Brain Date: Wed, 28 Feb 2024 11:13:30 +0100 Subject: [PATCH 1/4] Wrap tests around the current implementation --- .../deployment/v2/deployment_read.go | 6 +- .../deployment/v2/deployment_read_test.go | 57 +++++++++++++++++++ 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/ec/ecresource/deploymentresource/deployment/v2/deployment_read.go b/ec/ecresource/deploymentresource/deployment/v2/deployment_read.go index 8fbbd0622..a8c492178 100644 --- a/ec/ecresource/deploymentresource/deployment/v2/deployment_read.go +++ b/ec/ecresource/deploymentresource/deployment/v2/deployment_read.go @@ -21,10 +21,11 @@ import ( "context" "errors" "fmt" - "github.com/elastic/cloud-sdk-go/pkg/client/deployments" "slices" "strings" + "github.com/elastic/cloud-sdk-go/pkg/client/deployments" + "github.com/blang/semver" "github.com/elastic/cloud-sdk-go/pkg/models" "github.com/elastic/cloud-sdk-go/pkg/util/ec" @@ -271,8 +272,7 @@ func (dep *Deployment) parseCredentials(resources []*models.DeploymentResource) } func (dep *Deployment) ProcessSelfInObservability() { - - if dep.Observability == nil { + if dep == nil || dep.Observability == nil { return } diff --git a/ec/ecresource/deploymentresource/deployment/v2/deployment_read_test.go b/ec/ecresource/deploymentresource/deployment/v2/deployment_read_test.go index 12144a093..0a94a9140 100644 --- a/ec/ecresource/deploymentresource/deployment/v2/deployment_read_test.go +++ b/ec/ecresource/deploymentresource/deployment/v2/deployment_read_test.go @@ -28,8 +28,10 @@ import ( elasticsearchv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/elasticsearch/v2" enterprisesearchv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/enterprisesearch/v2" kibanav2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/kibana/v2" + observabilityv1 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/observability/v1" observabilityv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/observability/v2" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_readDeployment(t *testing.T) { @@ -1723,3 +1725,58 @@ func Test_getDeploymentTemplateID(t *testing.T) { }) } } + +func Test_ProcessSelfInObservability(t *testing.T) { + tests := []struct { + name string + deployment *Deployment + expectedObservabilityDeploymentID *string + }{ + { + name: "should noop if deployment is nil", + }, + { + name: "should noop if observability section is nil", + deployment: &Deployment{}, + }, + { + name: "should noop if observability deployment id is nil", + deployment: &Deployment{ + Observability: &observabilityv1.Observability{}, + }, + }, + { + name: "should set observability deployment id to _self_ if it equals the deployment id", + deployment: &Deployment{ + Id: "deployment-id", + Observability: &observabilityv1.Observability{ + DeploymentId: ec.String("deployment-id"), + }, + }, + expectedObservabilityDeploymentID: ec.String("self"), + }, + { + name: "should not change the observability deployment id if it does not equal the deployment id", + deployment: &Deployment{ + Id: "deployment-id", + Observability: &observabilityv1.Observability{ + DeploymentId: ec.String("another-deployment-id"), + }, + }, + expectedObservabilityDeploymentID: ec.String("another-deployment-id"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.deployment.ProcessSelfInObservability() + + var finalObservabilityDeploymentID *string + if tt.deployment != nil && tt.deployment.Observability != nil { + finalObservabilityDeploymentID = tt.deployment.Observability.DeploymentId + } + + require.Equal(t, tt.expectedObservabilityDeploymentID, finalObservabilityDeploymentID) + }) + } +} From 9569dd4d1d4b5f2c8e4361affe2f289640a3b97f Mon Sep 17 00:00:00 2001 From: Toby Brain Date: Wed, 28 Feb 2024 11:02:49 +0100 Subject: [PATCH 2/4] Don't overwrite the observability deployment id if it's explicitly configured to the current deployment id --- .../deployment/v2/deployment_read.go | 24 +++++++-- .../deployment/v2/deployment_read_test.go | 49 ++++++++++++++++++- ec/ecresource/deploymentresource/read.go | 2 +- 3 files changed, 69 insertions(+), 6 deletions(-) diff --git a/ec/ecresource/deploymentresource/deployment/v2/deployment_read.go b/ec/ecresource/deploymentresource/deployment/v2/deployment_read.go index a8c492178..d36f60378 100644 --- a/ec/ecresource/deploymentresource/deployment/v2/deployment_read.go +++ b/ec/ecresource/deploymentresource/deployment/v2/deployment_read.go @@ -35,12 +35,14 @@ import ( enterprisesearchv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/enterprisesearch/v2" integrationsserverv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/integrationsserver/v2" kibanav2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/kibana/v2" + v1 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/observability/v1" observabilityv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/observability/v2" "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/utils" "github.com/elastic/terraform-provider-ec/ec/internal/converters" "github.com/elastic/terraform-provider-ec/ec/internal/util" "github.com/hashicorp/terraform-plugin-framework/diag" "github.com/hashicorp/terraform-plugin-framework/types" + "github.com/hashicorp/terraform-plugin-framework/types/basetypes" ) type Deployment struct { @@ -271,18 +273,34 @@ func (dep *Deployment) parseCredentials(resources []*models.DeploymentResource) } } -func (dep *Deployment) ProcessSelfInObservability() { +func (dep *Deployment) ProcessSelfInObservability(ctx context.Context, base DeploymentTF) diag.Diagnostics { if dep == nil || dep.Observability == nil { - return + return nil } if dep.Observability.DeploymentId == nil { - return + return nil + } + + var baseObservability v1.ObservabilityTF + diags := base.Observability.As(ctx, &baseObservability, basetypes.ObjectAsOptions{ + UnhandledNullAsEmpty: true, + UnhandledUnknownAsEmpty: true, + }) + if diags.HasError() { + return diags + } + + deploymentIDIsKnown := !(baseObservability.DeploymentId.IsNull() || baseObservability.DeploymentId.IsUnknown()) + if deploymentIDIsKnown && baseObservability.DeploymentId.ValueString() != "self" { + return nil } if *dep.Observability.DeploymentId == dep.Id { *dep.Observability.DeploymentId = "self" } + + return nil } func (dep *Deployment) IncludePrivateStateTrafficFilters(ctx context.Context, base DeploymentTF, privateFilters []string) diag.Diagnostics { diff --git a/ec/ecresource/deploymentresource/deployment/v2/deployment_read_test.go b/ec/ecresource/deploymentresource/deployment/v2/deployment_read_test.go index 0a94a9140..3155597ee 100644 --- a/ec/ecresource/deploymentresource/deployment/v2/deployment_read_test.go +++ b/ec/ecresource/deploymentresource/deployment/v2/deployment_read_test.go @@ -18,6 +18,7 @@ package v2 import ( + "context" "errors" "testing" @@ -30,6 +31,8 @@ import ( kibanav2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/kibana/v2" observabilityv1 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/observability/v1" observabilityv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/observability/v2" + "github.com/hashicorp/terraform-plugin-framework/tfsdk" + "github.com/hashicorp/terraform-plugin-framework/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -1730,7 +1733,9 @@ func Test_ProcessSelfInObservability(t *testing.T) { tests := []struct { name string deployment *Deployment + baseObservability *observabilityv1.Observability expectedObservabilityDeploymentID *string + expectNonNilDiags bool }{ { name: "should noop if deployment is nil", @@ -1746,13 +1751,40 @@ func Test_ProcessSelfInObservability(t *testing.T) { }, }, { - name: "should set observability deployment id to _self_ if it equals the deployment id", + name: "should not change the observability deployment id to self if it equals the deployment id and the configured value is not self", deployment: &Deployment{ Id: "deployment-id", Observability: &observabilityv1.Observability{ DeploymentId: ec.String("deployment-id"), }, }, + baseObservability: &observabilityv1.Observability{ + DeploymentId: ec.String("deployment-id"), + }, + expectedObservabilityDeploymentID: ec.String("deployment-id"), + }, + { + name: "should set observability deployment id to self if it equals the deployment id and the configured value is self", + deployment: &Deployment{ + Id: "deployment-id", + Observability: &observabilityv1.Observability{ + DeploymentId: ec.String("deployment-id"), + }, + }, + baseObservability: &observabilityv1.Observability{ + DeploymentId: ec.String("self"), + }, + expectedObservabilityDeploymentID: ec.String("self"), + }, + { + name: "should set observability deployment id to self if it equals the deployment id and the configured value is not set", + deployment: &Deployment{ + Id: "deployment-id", + Observability: &observabilityv1.Observability{ + DeploymentId: ec.String("deployment-id"), + }, + }, + baseObservability: &observabilityv1.Observability{}, expectedObservabilityDeploymentID: ec.String("self"), }, { @@ -1769,7 +1801,20 @@ func Test_ProcessSelfInObservability(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - tt.deployment.ProcessSelfInObservability() + var obj types.Object + if tt.baseObservability != nil { + diags := tfsdk.ValueFrom(context.Background(), tt.baseObservability, observabilityv2.ObservabilitySchema().GetType(), &obj) + require.Nil(t, diags) + } + + baseDeployment := DeploymentTF{Observability: obj} + + diags := tt.deployment.ProcessSelfInObservability(context.Background(), baseDeployment) + if tt.expectNonNilDiags { + require.NotNil(t, diags) + } else { + require.Nil(t, diags) + } var finalObservabilityDeploymentID *string if tt.deployment != nil && tt.deployment.Observability != nil { diff --git a/ec/ecresource/deploymentresource/read.go b/ec/ecresource/deploymentresource/read.go index 32e3cb118..d5acf6a30 100644 --- a/ec/ecresource/deploymentresource/read.go +++ b/ec/ecresource/deploymentresource/read.go @@ -174,7 +174,7 @@ func (r *Resource) read(ctx context.Context, id string, state *deploymentv2.Depl deployment.SetCredentialsIfEmpty(state) - deployment.ProcessSelfInObservability() + diags.Append(deployment.ProcessSelfInObservability(ctx, base)...) deployment.NullifyUnusedEsTopologies(ctx, baseElasticsearch) From 8d8fd82fa39de2b866b13385a78ec64365be9e55 Mon Sep 17 00:00:00 2001 From: Toby Brain Date: Mon, 11 Mar 2024 18:21:43 +1100 Subject: [PATCH 3/4] Cover cases when observability is null or unknown --- .../deployment/v2/deployment_read_test.go | 52 +++++++++++++++---- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/ec/ecresource/deploymentresource/deployment/v2/deployment_read_test.go b/ec/ecresource/deploymentresource/deployment/v2/deployment_read_test.go index 3155597ee..956df847c 100644 --- a/ec/ecresource/deploymentresource/deployment/v2/deployment_read_test.go +++ b/ec/ecresource/deploymentresource/deployment/v2/deployment_read_test.go @@ -29,8 +29,8 @@ import ( elasticsearchv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/elasticsearch/v2" enterprisesearchv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/enterprisesearch/v2" kibanav2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/kibana/v2" - observabilityv1 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/observability/v1" observabilityv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/observability/v2" + "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/tfsdk" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/stretchr/testify/assert" @@ -1733,7 +1733,8 @@ func Test_ProcessSelfInObservability(t *testing.T) { tests := []struct { name string deployment *Deployment - baseObservability *observabilityv1.Observability + baseObservability *observabilityv2.Observability + observabilityIsUnknown bool expectedObservabilityDeploymentID *string expectNonNilDiags bool }{ @@ -1747,18 +1748,18 @@ func Test_ProcessSelfInObservability(t *testing.T) { { name: "should noop if observability deployment id is nil", deployment: &Deployment{ - Observability: &observabilityv1.Observability{}, + Observability: &observabilityv2.Observability{}, }, }, { name: "should not change the observability deployment id to self if it equals the deployment id and the configured value is not self", deployment: &Deployment{ Id: "deployment-id", - Observability: &observabilityv1.Observability{ + Observability: &observabilityv2.Observability{ DeploymentId: ec.String("deployment-id"), }, }, - baseObservability: &observabilityv1.Observability{ + baseObservability: &observabilityv2.Observability{ DeploymentId: ec.String("deployment-id"), }, expectedObservabilityDeploymentID: ec.String("deployment-id"), @@ -1767,11 +1768,11 @@ func Test_ProcessSelfInObservability(t *testing.T) { name: "should set observability deployment id to self if it equals the deployment id and the configured value is self", deployment: &Deployment{ Id: "deployment-id", - Observability: &observabilityv1.Observability{ + Observability: &observabilityv2.Observability{ DeploymentId: ec.String("deployment-id"), }, }, - baseObservability: &observabilityv1.Observability{ + baseObservability: &observabilityv2.Observability{ DeploymentId: ec.String("self"), }, expectedObservabilityDeploymentID: ec.String("self"), @@ -1780,31 +1781,60 @@ func Test_ProcessSelfInObservability(t *testing.T) { name: "should set observability deployment id to self if it equals the deployment id and the configured value is not set", deployment: &Deployment{ Id: "deployment-id", - Observability: &observabilityv1.Observability{ + Observability: &observabilityv2.Observability{ DeploymentId: ec.String("deployment-id"), }, }, - baseObservability: &observabilityv1.Observability{}, + baseObservability: &observabilityv2.Observability{}, expectedObservabilityDeploymentID: ec.String("self"), }, { name: "should not change the observability deployment id if it does not equal the deployment id", deployment: &Deployment{ Id: "deployment-id", - Observability: &observabilityv1.Observability{ + Observability: &observabilityv2.Observability{ DeploymentId: ec.String("another-deployment-id"), }, }, expectedObservabilityDeploymentID: ec.String("another-deployment-id"), }, + { + name: "should set observability deployment id to self if it equals the deployment id and no observability is configured", + deployment: &Deployment{ + Id: "deployment-id", + Observability: &observabilityv2.Observability{ + DeploymentId: ec.String("deployment-id"), + }, + }, + expectedObservabilityDeploymentID: ec.String("self"), + }, + { + name: "should set observability deployment id to self if it equals the deployment id and the configured value is unknown", + deployment: &Deployment{ + Id: "deployment-id", + Observability: &observabilityv2.Observability{ + DeploymentId: ec.String("deployment-id"), + }, + }, + observabilityIsUnknown: true, + expectedObservabilityDeploymentID: ec.String("self"), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var obj types.Object + observabilitySchema := observabilityv2.ObservabilitySchema().GetType() + schemaWithAttrs, ok := observabilitySchema.(attr.TypeWithAttributeTypes) + require.True(t, ok) + if tt.baseObservability != nil { - diags := tfsdk.ValueFrom(context.Background(), tt.baseObservability, observabilityv2.ObservabilitySchema().GetType(), &obj) + diags := tfsdk.ValueFrom(context.Background(), tt.baseObservability, observabilitySchema, &obj) require.Nil(t, diags) + } else if tt.observabilityIsUnknown { + obj = types.ObjectUnknown(schemaWithAttrs.AttributeTypes()) + } else { + obj = types.ObjectNull(schemaWithAttrs.AttributeTypes()) } baseDeployment := DeploymentTF{Observability: obj} From ca0ea28480fa6299476ad6dcf4c50c573d2bbd0d Mon Sep 17 00:00:00 2001 From: Toby Brain Date: Tue, 12 Mar 2024 12:06:41 +1100 Subject: [PATCH 4/4] Changelog --- .changelog/789.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/789.txt diff --git a/.changelog/789.txt b/.changelog/789.txt new file mode 100644 index 000000000..ac2e5eec3 --- /dev/null +++ b/.changelog/789.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/deployment: Don't rewrite the observability deployment ID to `self` when it's been explicitly configured. +```