Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Implements tags and labels as MapAttribute in advanced_cluster schema v2 #2996

Merged
merged 16 commits into from
Jan 24, 2025
Merged

chore: Implements tags and labels as MapAttribute in advanced_cluster schema v2 #2996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
623161a
schema change
lantoli Jan 22, 2025
214906b
model change
lantoli Jan 22, 2025
8fd6649
create TF models
lantoli Jan 23, 2025
99cde29
create Admin models
lantoli Jan 23, 2025
fdc1bf0
labels & tags conversion
lantoli Jan 23, 2025
3653edc
TEMPORARY skip mocked tests
lantoli Jan 23, 2025
241f8b0
Merge branch 'master' into CLOUDP-295198_tpf_tags_labels
lantoli Jan 23, 2025
360c86e
handle null vs []
lantoli Jan 23, 2025
e150067
Revert "TEMPORARY skip mocked tests"
lantoli Jan 23, 2025
86314a9
Merge branch 'master' into CLOUDP-295198_tpf_tags_labels
lantoli Jan 24, 2025
bf19829
temporary don't test check label and tags
lantoli Jan 24, 2025
c3aceeb
improve update and override
lantoli Jan 24, 2025
1198109
fix plan changes in state upgrade
lantoli Jan 24, 2025
0067508
test checks for labels and tags
lantoli Jan 24, 2025
35900d4
Merge branch 'master' into CLOUDP-295198_tpf_tags_labels
lantoli Jan 24, 2025
f24f8dc
apply feedback
lantoli Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 55 additions & 24 deletions internal/service/advancedcluster/resource_advanced_cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,15 +590,15 @@ func TestAccClusterAdvancedCluster_withTags(t *testing.T) {
Steps: []resource.TestStep{
{
Config: configWithKeyValueBlocks(t, true, orgID, projectName, clusterName, "tags"),
Check: checkKeyValueBlocks(true, clusterName, "tags"),
Check: checkKeyValueBlocks(true, true, "tags"),
},
{
Config: configWithKeyValueBlocks(t, true, orgID, projectName, clusterName, "tags", acc.ClusterTagsMap1, acc.ClusterTagsMap2),
Check: checkKeyValueBlocks(true, clusterName, "tags", acc.ClusterTagsMap1, acc.ClusterTagsMap2),
Check: checkKeyValueBlocks(true, true, "tags", acc.ClusterTagsMap1, acc.ClusterTagsMap2),
},
{
Config: configWithKeyValueBlocks(t, true, orgID, projectName, clusterName, "tags", acc.ClusterTagsMap3),
Check: checkKeyValueBlocks(true, clusterName, "tags", acc.ClusterTagsMap3),
Check: checkKeyValueBlocks(true, true, "tags", acc.ClusterTagsMap3),
},
},
})
Expand All @@ -618,15 +618,15 @@ func TestAccClusterAdvancedCluster_withLabels(t *testing.T) {
Steps: []resource.TestStep{
{
Config: configWithKeyValueBlocks(t, true, orgID, projectName, clusterName, "labels"),
Check: checkKeyValueBlocks(true, clusterName, "labels"),
Check: checkKeyValueBlocks(true, true, "labels"),
},
{
Config: configWithKeyValueBlocks(t, true, orgID, projectName, clusterName, "labels", acc.ClusterLabelsMap1, acc.ClusterLabelsMap2),
Check: checkKeyValueBlocks(true, clusterName, "labels", acc.ClusterLabelsMap1, acc.ClusterLabelsMap2),
Check: checkKeyValueBlocks(true, true, "labels", acc.ClusterLabelsMap1, acc.ClusterLabelsMap2),
},
{
Config: configWithKeyValueBlocks(t, true, orgID, projectName, clusterName, "labels", acc.ClusterLabelsMap3),
Check: checkKeyValueBlocks(true, clusterName, "labels", acc.ClusterLabelsMap3),
Check: checkKeyValueBlocks(true, true, "labels", acc.ClusterLabelsMap3),
},
},
})
Expand Down Expand Up @@ -1134,15 +1134,14 @@ func TestAccMockableAdvancedCluster_replicasetAdvConfigUpdate(t *testing.T) {
"replication_specs.0.container_id.AWS:US_EAST_1",
}
timeoutCheck = resource.TestCheckResourceAttr(resourceName, "timeouts.create", "6000s") // timeouts.create is not set on data sources
tagsLabelsMap = map[string]string{"key": "env", "value": "test"}
tagsCheck = checkKeyValueBlocks(true, false, "tags", tagsLabelsMap)
labelsCheck = checkKeyValueBlocks(true, false, "labels", tagsLabelsMap)
checks = checkAggr(true, checksSet, checksMap, timeoutCheck)
afterUpdateMap = map[string]string{
"state_name": "IDLE",
"backup_enabled": "true",
"bi_connector_config.0.enabled": "true",
"labels.0.key": "env",
"labels.0.value": "test",
"tags.0.key": "env",
"tags.0.value": "test",
"mongo_db_major_version": "8.0",
"pit_enabled": "true",
"redact_client_log_data": "true",
Expand All @@ -1162,7 +1161,7 @@ func TestAccMockableAdvancedCluster_replicasetAdvConfigUpdate(t *testing.T) {
"advanced_configuration.0.custom_openssl_cipher_config_tls12.#": "1",
"advanced_configuration.0.default_max_time_ms": "65",
}
checksUpdate = checkAggr(true, checksSet, afterUpdateMap, timeoutCheck)
checksUpdate = checkAggr(true, checksSet, afterUpdateMap, timeoutCheck, tagsCheck, labelsCheck)
fullUpdate = `
backup_enabled = true
bi_connector_config {
Expand Down Expand Up @@ -1479,28 +1478,60 @@ func configWithKeyValueBlocks(t *testing.T, isAcc bool, orgID, projectName, clus
`, orgID, projectName, clusterName, extraConfig)) + dataSourcesTFNewSchema
}

func checkKeyValueBlocks(isAcc bool, clusterName, blockName string, blocks ...map[string]string) resource.TestCheckFunc {
func checkKeyValueBlocks(isAcc, includeDataSources bool, blockName string, blocks ...map[string]string) resource.TestCheckFunc {
if config.AdvancedClusterV2Schema() {
return checkKeyValueBlocksSchemaV2(isAcc, includeDataSources, blockName, blocks...)
}
const pluralPrefix = "results.0."
lenStr := strconv.Itoa(len(blocks))
keyHash := fmt.Sprintf("%s.#", blockName)
keyStar := fmt.Sprintf("%s.*", blockName)
keyHash := blockName + ".#"
keyStar := blockName + ".*"
checks := []resource.TestCheckFunc{
acc.TestCheckResourceAttrSchemaV2(isAcc, resourceName, keyHash, lenStr),
acc.TestCheckResourceAttrSchemaV2(isAcc, dataSourceName, keyHash, lenStr),
acc.TestCheckResourceAttrSchemaV2(isAcc, dataSourcePluralName, pluralPrefix+keyHash, lenStr),
}
if includeDataSources {
checks = append(checks,
acc.TestCheckResourceAttrSchemaV2(isAcc, dataSourceName, keyHash, lenStr),
acc.TestCheckResourceAttrSchemaV2(isAcc, dataSourcePluralName, pluralPrefix+keyHash, lenStr))
}
for _, block := range blocks {
checks = append(checks,
acc.TestCheckTypeSetElemNestedAttrsSchemaV2(isAcc, resourceName, keyStar, block),
acc.TestCheckTypeSetElemNestedAttrsSchemaV2(isAcc, dataSourceName, keyStar, block),
acc.TestCheckTypeSetElemNestedAttrsSchemaV2(isAcc, dataSourcePluralName, pluralPrefix+keyStar, block))
)
if includeDataSources {
checks = append(checks,
acc.TestCheckTypeSetElemNestedAttrsSchemaV2(isAcc, dataSourceName, keyStar, block),
acc.TestCheckTypeSetElemNestedAttrsSchemaV2(isAcc, dataSourcePluralName, pluralPrefix+keyStar, block))
}
}
return checkAggr(isAcc,
[]string{"project_id"},
map[string]string{
"name": clusterName,
},
checks...)
return resource.ComposeAggregateTestCheckFunc(checks...)
}

func checkKeyValueBlocksSchemaV2(isAcc, includeDataSources bool, blockName string, blocks ...map[string]string) resource.TestCheckFunc {
const pluralPrefix = "results.0."
lenStr := strconv.Itoa(len(blocks))
keyPct := blockName + ".%"
checks := []resource.TestCheckFunc{
acc.TestCheckResourceAttrSchemaV2(isAcc, resourceName, keyPct, lenStr),
}
if includeDataSources {
checks = append(checks,
acc.TestCheckResourceAttrSchemaV2(isAcc, dataSourceName, keyPct, lenStr),
acc.TestCheckResourceAttrSchemaV2(isAcc, dataSourcePluralName, pluralPrefix+keyPct, lenStr))
}
for _, block := range blocks {
key := blockName + "." + block["key"]
value := block["value"]
checks = append(checks,
acc.TestCheckResourceAttrSchemaV2(isAcc, resourceName, key, value),
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @EspenAlbert for the tips for checking maps

if includeDataSources {
checks = append(checks,
acc.TestCheckResourceAttrSchemaV2(isAcc, dataSourceName, key, value),
acc.TestCheckResourceAttrSchemaV2(isAcc, dataSourcePluralName, pluralPrefix+key, value))
}
}
return resource.ComposeAggregateTestCheckFunc(checks...)
}

func configReplicaSetAWSProvider(t *testing.T, isAcc bool, projectID, name string, diskSizeGB, nodeCountElectable int) string {
Expand Down
48 changes: 19 additions & 29 deletions internal/service/advancedclustertpf/model_ClusterDescription20240805.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ type ExtraAPIInfo struct {
func NewTFModel(ctx context.Context, input *admin.ClusterDescription20240805, timeout timeouts.Value, diags *diag.Diagnostics, apiInfo ExtraAPIInfo) *TFModel {
biConnector := NewBiConnectorConfigObjType(ctx, input.BiConnector, diags)
connectionStrings := NewConnectionStringsObjType(ctx, input.ConnectionStrings, diags)
labels := NewLabelsObjType(ctx, input.Labels, diags)
labels := NewLabelsObjType(ctx, diags, input.Labels)
replicationSpecs := NewReplicationSpecsObjType(ctx, input.ReplicationSpecs, diags, &apiInfo)
tags := NewTagsObjType(ctx, input.Tags, diags)
tags := NewTagsObjType(ctx, diags, input.Tags)
pinnedFCV := NewPinnedFCVObjType(ctx, input, diags)
if diags.HasError() {
return nil
Expand Down Expand Up @@ -100,22 +100,19 @@ func NewConnectionStringsObjType(ctx context.Context, input *admin.ClusterConnec
return objType
}

func NewLabelsObjType(ctx context.Context, input *[]admin.ComponentLabel, diags *diag.Diagnostics) types.Set {
if input == nil {
return types.SetNull(LabelsObjType)
}
tfModels := make([]TFLabelsModel, 0, len(*input))
for _, item := range *input {
key := conversion.SafeValue(item.Key)
value := conversion.SafeValue(item.Value)
if key == LegacyIgnoredLabelKey {
continue
func NewLabelsObjType(ctx context.Context, diags *diag.Diagnostics, input *[]admin.ComponentLabel) types.Map {
elms := make(map[string]string)
if input != nil {
for _, item := range *input {
key := item.GetKey()
value := item.GetValue()
if key == LegacyIgnoredLabelKey {
continue
}
elms[key] = value
}
tfModels = append(tfModels, TFLabelsModel{Key: types.StringValue(key), Value: types.StringValue(value)})
}
setType, diagsLocal := types.SetValueFrom(ctx, LabelsObjType, tfModels)
diags.Append(diagsLocal...)
return setType
return conversion.ToTFMapOfString(ctx, diags, &elms)
}

func NewReplicationSpecsObjType(ctx context.Context, input *[]admin.ReplicationSpec20240805, diags *diag.Diagnostics, apiInfo *ExtraAPIInfo) types.List {
Expand Down Expand Up @@ -235,21 +232,14 @@ func convertReplicationSpecsLegacy(ctx context.Context, input *[]admin.Replicati
return &tfModels
}

func NewTagsObjType(ctx context.Context, input *[]admin.ResourceTag, diags *diag.Diagnostics) types.Set {
if input == nil {
// API Response not consistent, even when not set in POST/PATCH `[]` is returned instead of null
return types.SetValueMust(TagsObjType, nil)
}
tfModels := make([]TFTagsModel, len(*input))
for i, item := range *input {
tfModels[i] = TFTagsModel{
Key: types.StringValue(item.Key),
Value: types.StringValue(item.Value),
func NewTagsObjType(ctx context.Context, diags *diag.Diagnostics, input *[]admin.ResourceTag) types.Map {
elms := make(map[string]string)
if input != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice refactor!

for _, item := range *input {
elms[item.GetKey()] = item.GetValue()
}
}
setType, diagsLocal := types.SetValueFrom(ctx, TagsObjType, tfModels)
diags.Append(diagsLocal...)
return setType
return conversion.ToTFMapOfString(ctx, diags, &elms)
}

func NewPrivateEndpointObjType(ctx context.Context, input *[]admin.ClusterDescriptionConnectionStringsPrivateEndpoint, diags *diag.Diagnostics) types.List {
Expand Down
63 changes: 30 additions & 33 deletions internal/service/advancedclustertpf/model_to_ClusterDescription20240805.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func NewAtlasReq(ctx context.Context, input *TFModel, diags *diag.Diagnostics) *
ConfigServerManagementMode: conversion.NilForUnknown(input.ConfigServerManagementMode, input.ConfigServerManagementMode.ValueStringPointer()),
EncryptionAtRestProvider: conversion.NilForUnknown(input.EncryptionAtRestProvider, input.EncryptionAtRestProvider.ValueStringPointer()),
GlobalClusterSelfManagedSharding: conversion.NilForUnknown(input.GlobalClusterSelfManagedSharding, input.GlobalClusterSelfManagedSharding.ValueBoolPointer()),
Labels: newComponentLabel(ctx, input.Labels, diags),
Labels: newComponentLabel(ctx, diags, input.Labels),
MongoDBMajorVersion: majorVersion,
Name: input.Name.ValueStringPointer(),
Paused: conversion.NilForUnknown(input.Paused, input.Paused.ValueBoolPointer()),
Expand All @@ -40,7 +40,7 @@ func NewAtlasReq(ctx context.Context, input *TFModel, diags *diag.Diagnostics) *
ReplicaSetScalingStrategy: conversion.NilForUnknown(input.ReplicaSetScalingStrategy, input.ReplicaSetScalingStrategy.ValueStringPointer()),
ReplicationSpecs: newReplicationSpec20240805(ctx, input.ReplicationSpecs, diags),
RootCertType: conversion.NilForUnknown(input.RootCertType, input.RootCertType.ValueStringPointer()),
Tags: newResourceTag(ctx, input.Tags, diags),
Tags: newResourceTag(ctx, diags, input.Tags),
TerminationProtectionEnabled: conversion.NilForUnknown(input.TerminationProtectionEnabled, input.TerminationProtectionEnabled.ValueBoolPointer()),
VersionReleaseSystem: conversion.NilForUnknown(input.VersionReleaseSystem, input.VersionReleaseSystem.ValueStringPointer()),
}
Expand All @@ -60,29 +60,28 @@ func newBiConnector(ctx context.Context, input types.Object, diags *diag.Diagnos
ReadPreference: conversion.NilForUnknown(item.ReadPreference, item.ReadPreference.ValueStringPointer()),
}
}
func newComponentLabel(ctx context.Context, input types.Set, diags *diag.Diagnostics) *[]admin.ComponentLabel {
if input.IsUnknown() {
return nil
}
elements := make([]TFLabelsModel, len(input.Elements()))
if localDiags := input.ElementsAs(ctx, &elements, false); len(localDiags) > 0 {
diags.Append(localDiags...)

func newComponentLabel(ctx context.Context, diags *diag.Diagnostics, input types.Map) *[]admin.ComponentLabel {
elms := make(map[string]types.String, len(input.Elements()))
localDiags := input.ElementsAs(ctx, &elms, false)
diags.Append(localDiags...)
if diags.HasError() {
return nil
}
resp := make([]admin.ComponentLabel, len(input.Elements()))
for i := range elements {
item := &elements[i]
if item.Key.ValueString() == LegacyIgnoredLabelKey {
ret := make([]admin.ComponentLabel, 0, len(input.Elements()))
for key, value := range elms {
if key == LegacyIgnoredLabelKey {
diags.AddError(ErrLegacyIgnoreLabel.Error(), ErrLegacyIgnoreLabel.Error())
return nil
}
resp[i] = admin.ComponentLabel{
Key: item.Key.ValueStringPointer(),
Value: item.Value.ValueStringPointer(),
}
ret = append(ret, admin.ComponentLabel{
Key: &key,
Value: value.ValueStringPointer(),
})
}
return &resp
return &ret
}

func newReplicationSpec20240805(ctx context.Context, input types.List, diags *diag.Diagnostics) *[]admin.ReplicationSpec20240805 {
if input.IsUnknown() || input.IsNull() {
return nil
Expand Down Expand Up @@ -113,25 +112,23 @@ func resolveZoneNameOrUseDefault(item *TFReplicationSpecsModel) string {
return *zoneName
}

func newResourceTag(ctx context.Context, input types.Set, diags *diag.Diagnostics) *[]admin.ResourceTag {
if input.IsUnknown() {
func newResourceTag(ctx context.Context, diags *diag.Diagnostics, input types.Map) *[]admin.ResourceTag {
elms := make(map[string]types.String, len(input.Elements()))
localDiags := input.ElementsAs(ctx, &elms, false)
diags.Append(localDiags...)
if diags.HasError() {
return nil
}
elements := make([]TFTagsModel, len(input.Elements()))
if localDiags := input.ElementsAs(ctx, &elements, false); len(localDiags) > 0 {
diags.Append(localDiags...)
return nil
ret := make([]admin.ResourceTag, 0, len(input.Elements()))
for key, value := range elms {
ret = append(ret, admin.ResourceTag{
Key: key,
Value: value.ValueString(),
})
}
resp := make([]admin.ResourceTag, len(input.Elements()))
for i := range elements {
item := &elements[i]
resp[i] = admin.ResourceTag{
Key: item.Key.ValueString(),
Value: item.Value.ValueString(),
}
}
return &resp
return &ret
}

func newCloudRegionConfig20240805(ctx context.Context, input types.List, diags *diag.Diagnostics) *[]admin.CloudRegionConfig20240805 {
if input.IsUnknown() || input.IsNull() {
return nil
Expand Down
5 changes: 5 additions & 0 deletions internal/service/advancedclustertpf/move_upgrade_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@ func setStateResponse(ctx context.Context, diags *diag.Diagnostics, stateIn *tfp
return
}
setOptionalModelAttrs(ctx, stateObj, model)

// Set tags and labels to null instead of empty so there is no plan change if there are no tags or labels when Read is called.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you had to learn this the hard way? If you do nothing they are set to empty map? {}?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha ha, exactly

model.Tags = types.MapNull(types.StringType)
model.Labels = types.MapNull(types.StringType)

diags.Append(stateOut.Set(ctx, model)...)
}

Expand Down
7 changes: 5 additions & 2 deletions internal/service/advancedclustertpf/resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,11 @@ func (r *rs) Update(ctx context.Context, req resource.UpdateRequest, resp *resou
if diags.HasError() {
return
}
modelOut := &state
if clusterResp != nil {
var modelOut *TFModel
if clusterResp == nil { // no Atlas updates needed but override is still needed (e.g. tags going from nil to [] or vice versa)
modelOut = &state
overrideAttributesWithPrevStateValue(&plan, modelOut)
} else {
modelOut, _ = getBasicClusterModel(ctx, diags, r.Client, clusterResp, &plan, false)
if diags.HasError() {
return
Expand Down
12 changes: 12 additions & 0 deletions internal/service/advancedclustertpf/resource_compatiblity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,18 @@ func overrideAttributesWithPrevStateValue(modelIn, modelOut *TFModel) {
if retainBackups != nil && !modelIn.RetainBackupsEnabled.Equal(modelOut.RetainBackupsEnabled) {
modelOut.RetainBackupsEnabled = types.BoolPointerValue(retainBackups)
}
overrideMapStringWithPrevStateValue(&modelIn.Labels, &modelOut.Labels)
overrideMapStringWithPrevStateValue(&modelIn.Tags, &modelOut.Tags)
}
func overrideMapStringWithPrevStateValue(mapIn, mapOut *types.Map) {
Comment on lines +25 to +28
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see for cases of the empty lists/map we sometimes go for setting nil directly (example) or in cases like this one support defining the empty map in the config. I would assess what is the drawback UX wise if we dont have this logic to avoid relying on previous state is not ideal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed offline

if mapIn == nil || mapOut == nil || len(mapOut.Elements()) > 0 {
return
}
if mapIn.IsNull() {
*mapOut = types.MapNull(types.StringType)
} else {
*mapOut = types.MapValueMust(types.StringType, nil)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice usage of Must

}
}

func findNumShardsUpdates(ctx context.Context, state, plan *TFModel, diags *diag.Diagnostics) map[string]int64 {
Expand Down
Loading
Loading