chore: Bring pinned_fcv feature into advanced_cluster TPF implementation
#2970
Conversation
| sdkv2diag "github.com/hashicorp/terraform-plugin-sdk/v2/diag" | ||
| ) | ||
|
|
||
| func FromTPFDiagsToSDKV2Diags(diagsTpf []diag.Diagnostic) sdkv2diag.Diagnostics { |
There was a problem hiding this comment.
i like the idea of this type of funcs to transform from TFP responses back to SDKv2 (and the opposite for request attrs)
| return modelOut | ||
| } | ||
|
|
||
| func (r *rs) applyPinnedFCVChanges(ctx context.Context, diags *diag.Diagnostics, state, plan *TFModel) *admin.ClusterDescription20240805 { |
There was a problem hiding this comment.
i guess this main flow logic can't be made common
There was a problem hiding this comment.
correct, I think if we want to extract further we would have to start thinking of a common model that is agnostic of either TFModel (tpf) or ResourceData (sdk)
| return "" | ||
| } | ||
|
|
||
| func PinFCV(ctx context.Context, api admin.ClustersApi, projectID, clusterName, expirationDateStr string) error { |
There was a problem hiding this comment.
this common logic is great! i guess no more common logic can be extracted at the moment
| } | ||
|
|
||
| func CheckRSAndDSSchemaV2(isAcc bool, resourceName string, dataSourceName, pluralDataSourceName *string, attrsSet []string, attrsMap map[string]string, extra ...resource.TestCheckFunc) resource.TestCheckFunc { | ||
| checks := []resource.TestCheckFunc{} |
There was a problem hiding this comment.
can CheckRSAndDSSchemaV2 implementation rely on CheckRSAndDS and be in advanced_cluster_schema_v2.go?
e.g.
func CheckRSAndDSSchemaV2(isAcc bool, resourceName string, dataSourceName, pluralDataSourceName *string, attrsSet []string, attrsMap map[string]string, extra ...resource.TestCheckFunc) resource.TestCheckFunc {
return CheckRSAndDS(resourceName, dataSourceName, pluralDataSourceName, ConvertToSchemaV2AttrsSet(isAcc, attrsSet), ConvertToSchemaV2AttrsMap(isAcc, attrsMap), extra...)
}
There was a problem hiding this comment.
great suggestion, had not thought of this approach. Adjusted.
| } | ||
|
|
||
| func configFCVPinning(orgID, projectName, clusterName string, pinningExpirationDate *string, mongoDBMajorVersion string) string { | ||
| func configFCVPinning(t *testing.T, isAcc bool, orgID, projectName, clusterName string, pinningExpirationDate *string, mongoDBMajorVersion string) string { |
There was a problem hiding this comment.
are we planning a migration test? (why is isAcc introduced)
There was a problem hiding this comment.
good question, not really, will remove the parameter for now and if we eventually have the need the parameter can be added to the config function.
| return resource.ComposeAggregateTestCheckFunc(checks...) | ||
| extraChecks := extra | ||
| extraChecks = append(extraChecks, acc.CheckExistsCluster(resourceName)) | ||
| return acc.CheckRSAndDSSchemaV2(isAcc, resourceName, admin.PtrString(dataSourceName), nil, attrsSet, attrsMap, extraChecks...) |
|
Ran tests against qa to verify pinned fcv tests are passing |
* master: chore: Bring `pinned_fcv` feature into advanced_cluster TPF implementation (#2970)
3 participants
Description
Link to any related issue(s): CLOUDP-291160
Recap of changes made in PR:
advancedclusterto also run withadvancedclustertpfimplementation.advancedclustertpfextracting common logic where possible, specifically pinning logic and generation of warning messages.Known TPF vs SDK v2 differences:
pinned_fcv[0].versionvspinned_fcv.version) as well as its null value ([]vsnull).Type of change:
Required Checklist:
Further comments