test: Updates importIgnoredFields function to handle dynamic replication specs #2977
Conversation
| func importIgnoredFields() []string { | ||
| func importIgnoredFields(replicationSpecs int) []string { | ||
| if config.AdvancedClusterV2Schema() { | ||
| return []string{} |
There was a problem hiding this comment.
nit: i think you can just return nil here
| "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{} |
There was a problem hiding this comment.
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
| } | ||
|
|
||
| func importIgnoredFields() []string { | ||
| func importIgnoredFields(replicationSpecs int) []string { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
doc added and refactor, please see the latest code
| // However, as import doesn't have a previous state to compare with, import will always fill them. | ||
| // This will make these fields differ in the state, although the plan change won't be shown to the user as they're computed values. | ||
| if !config.AdvancedClusterV2Schema() { | ||
| ignorePrefixFields = append(ignorePrefixFields, "replication_specs") |
There was a problem hiding this comment.
This is quite broad. But I like that you could use the TestStepImportCluster instead of the extra helper
There was a problem hiding this comment.
@EspenAlbert it is for SDKv2 tests, but reality is that any spec that is not in the definition file must be included, so at then it's like playing to exclude the elements that are not in the definition file.
however it's more restrict for TPF now, because some tests had "replication_specs" for SDKv2 and TPF, not this is only for SDKv2 tests.
3 participants
Description
Updates importIgnoredFields function to handle dynamic replication specs
Type of change:
Required Checklist:
Further comments