Skip to content

test: Updates importIgnoredFields function to handle dynamic replication specs #2977

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

Merged
merged 7 commits into from
Jan 20, 2025
Merged

test: Updates importIgnoredFields function to handle dynamic replication specs #2977

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
24b0e97
test: Updates importIgnoredFields function to handle dynamic replicat…
EspenAlbert Jan 17, 2025
a3c5dc5
improve naming, reverse condition
lantoli Jan 17, 2025
2426d2e
ignore all changing attributes
lantoli Jan 17, 2025
08b40ee
clarify tests
lantoli Jan 19, 2025
781aa1a
refactor acc.TestStepImportCluster
lantoli Jan 19, 2025
29fe715
revert unrelated changes
lantoli Jan 19, 2025
f64edbb
ignore all replication_specs
lantoli Jan 20, 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
18 changes: 11 additions & 7 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 @@ -1190,7 +1190,7 @@ func TestAccMockableAdvancedCluster_replicasetAdvConfigUpdate(t *testing.T) {
Config: configBasicReplicaset(t, projectID, clusterName, fullUpdate),
Check: checksUpdate,
},
acc.TestStepImportCluster(resourceName, importIgnoredFields()...),
acc.TestStepImportCluster(resourceName, importIgnoredFields(1)...),
},
})
}
Expand Down Expand Up @@ -1233,7 +1233,7 @@ func TestAccMockableAdvancedCluster_shardedAddAnalyticsAndAutoScaling(t *testing
Config: configSharded(t, projectID, clusterName, true),
Check: checksUpdated,
},
acc.TestStepImportCluster(resourceName, importIgnoredFields()...),
acc.TestStepImportCluster(resourceName, importIgnoredFields(2)...),
},
})
}
Expand Down Expand Up @@ -2782,13 +2782,17 @@ func configFCVPinning(t *testing.T, orgID, projectName, clusterName string, pinn
`, orgID, projectName, clusterName, mongoDBMajorVersion, pinnedFCVAttr)) + dataSourcesTFNewSchema
}

func importIgnoredFields() []string {
func importIgnoredFields(replicationSpecs int) []string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why these ignored methods and why adding now the int?

I am not seeing a lot of documentation and this code is going to be very hard to follow and understand why in few months

Copy link
Member

@lantoli lantoli Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

note that it has a AdvancedClusterV2Schema clause. It's because SDKv2 doesn't fill those fields (we talked about this in yesterday's meeting) so we don't want to check those attributes only for TPF. Maybe a better param name would be replicationSpecCount, to specify how many replicationSpecs are, so we want to ignore those attribute changes in import tests, as we know they happen because SDKv2 and TPF fill them differently.

these attribute checks are new to TPF, were not being done in SDKv2

Copy link
Member

@lantoli lantoli Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

doc added and refactor, please see the latest code

if config.AdvancedClusterV2Schema() {
return []string{}
Copy link
Member

Choose a reason for hiding this comment

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

nit: i think you can just return nil here

}
return []string{
"replication_specs.0.region_configs.0.read_only_specs",
"replication_specs.0.region_configs.0.analytics_specs",
"replication_specs.0.region_configs.0.electable_specs.0.ebs_volume_type",
ignored := []string{}
Copy link
Member

Choose a reason for hiding this comment

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

nit: similarly, you don't need to initialize, can just do:

var ignored []string

nil slices are in general equivalent to empy (non-nil) ones

for i := range replicationSpecs {
ignored = append(ignored,
fmt.Sprintf("replication_specs.%d.region_configs.0.read_only_specs", i),
fmt.Sprintf("replication_specs.%d.region_configs.0.analytics_specs", i),
fmt.Sprintf("replication_specs.%d.region_configs.0.electable_specs.0.ebs_volume_type", i),
)
}
return ignored
}
Loading