Skip to content

Commit

Permalink
Dont overwrite deployment id with self (#789)
Browse files Browse the repository at this point in the history
* Wrap tests around the current implementation

* Don't overwrite the observability deployment id if it's explicitly configured to the current deployment id

* Cover cases when observability is null or unknown

* Changelog
  • Loading branch information
tobio authored Mar 12, 2024
1 parent cc5eba7 commit a77efc4
Show file tree
Hide file tree
Showing 4 changed files with 153 additions and 6 deletions.
3 changes: 3 additions & 0 deletions .changelog/789.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
resource/deployment: Don't rewrite the observability deployment ID to `self` when it's been explicitly configured.
```
27 changes: 22 additions & 5 deletions ec/ecresource/deploymentresource/deployment/v2/deployment_read.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,15 @@ 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/tfsdk"
"github.com/hashicorp/terraform-plugin-framework/types"
"github.com/hashicorp/terraform-plugin-framework/types/basetypes"
)

type Deployment struct {
Expand Down Expand Up @@ -295,19 +297,34 @@ func (dep *Deployment) parseCredentials(resources []*models.DeploymentResource)
}
}

func (dep *Deployment) ProcessSelfInObservability() {

if dep.Observability == nil {
return
func (dep *Deployment) ProcessSelfInObservability(ctx context.Context, base DeploymentTF) diag.Diagnostics {
if dep == nil || dep.Observability == nil {
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 {
Expand Down
127 changes: 127 additions & 0 deletions ec/ecresource/deploymentresource/deployment/v2/deployment_read_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
enterprisesearchv2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/enterprisesearch/v2"
kibanav2 "github.com/elastic/terraform-provider-ec/ec/ecresource/deploymentresource/kibana/v2"
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"
Expand Down Expand Up @@ -1729,6 +1730,132 @@ func Test_getDeploymentTemplateID(t *testing.T) {
}
}

func Test_ProcessSelfInObservability(t *testing.T) {
tests := []struct {
name string
deployment *Deployment
baseObservability *observabilityv2.Observability
observabilityIsUnknown bool
expectedObservabilityDeploymentID *string
expectNonNilDiags bool
}{
{
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: &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: &observabilityv2.Observability{
DeploymentId: ec.String("deployment-id"),
},
},
baseObservability: &observabilityv2.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: &observabilityv2.Observability{
DeploymentId: ec.String("deployment-id"),
},
},
baseObservability: &observabilityv2.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: &observabilityv2.Observability{
DeploymentId: ec.String("deployment-id"),
},
},
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: &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, 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}

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 {
finalObservabilityDeploymentID = tt.deployment.Observability.DeploymentId
}

require.Equal(t, tt.expectedObservabilityDeploymentID, finalObservabilityDeploymentID)
})
}
}

func Test_PersistSnapshotSource(t *testing.T) {
tests := []struct {
name string
Expand Down
2 changes: 1 addition & 1 deletion ec/ecresource/deploymentresource/read.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
diags.Append(deployment.PersistSnapshotSource(ctx, baseElasticsearch)...)
Expand Down

0 comments on commit a77efc4

Please sign in to comment.