GoogleCloudPlatform
Proposal: use embedded structs to add GetProject methods to PF-implemented resources & data sources
Blocked by muxing fixes PR (this PR contains that PR's commits)
Background
Dating from the time of the original muxing in 2023 there is a function called GetProjectFramework that's used to determine whether to take the project value from a resource argument or from the provider default.
// GetProject reads the "project" field from the given resource and falls
// back to the provider's value if not given. If the provider's value is not
// given, an error is returned.
func GetProjectFramework(rVal, pVal types.String, diags *diag.Diagnostics) types.String {
return getProjectFromFrameworkSchema("project", rVal, pVal, diags)
}
func getProjectFromFrameworkSchema(projectSchemaField string, rVal, pVal types.String, diags *diag.Diagnostics) types.String {
if !rVal.IsNull() && rVal.ValueString() != "" {
return rVal
}
if !pVal.IsNull() && pVal.ValueString() != "" {
return pVal
}
diags.AddError("required field is not set", fmt.Sprintf("%s is not set", projectSchemaField))
return types.String{}
}
This was written to heavily mimic the original, SDKv2 equivalent GetProject function. This function needed a 'schema field' argument because the first argument was *schema.ResourceData, the entire data for a resource. The function needed instructions on what key-value pair to look for and retrieve from that data structure:
// GetProject reads the "project" field from the given resource data and falls
// back to the provider's value if not given. If the provider's value is not
// given, an error is returned.
func GetProject(d TerraformResourceData, config *transport_tpg.Config) (string, error) {
return GetProjectFromSchema("project", d, config)
}
func GetProjectFromSchema(projectSchemaField string, d TerraformResourceData, config *transport_tpg.Config) (string, error) {
res, ok := d.GetOk(projectSchemaField)
if ok && projectSchemaField != "" {
return res.(string), nil
}
if config.Project != "" {
return config.Project, nil
}
return "", fmt.Errorf("%s: required field is not set", projectSchemaField)
}
My opinion
Mimicking the original SDK code when implementing a plugin-framework version of this logic doesn't make sense, as a resource's data is no longer stored as a map[string]interface{} matching a schema which can be queried with string keys. Instead the data is read into a struct annotated with struct tags that link fields to config values. For example:
type GoogleFirebaseAppleAppConfigModel struct {
Id types.String `tfsdk:"id"`
AppId types.String `tfsdk:"app_id"`
ConfigFilename types.String `tfsdk:"config_filename"`
ConfigFileContents types.String `tfsdk:"config_file_contents"`
Project types.String `tfsdk:"project"`
}
Given this, there isn't any sense passing in the string "project" to getProjectFromFrameworkSchema above. And if you remove that part of a logic you end up with a generic function that compares two arguments using the new type system and then return the first one that isn't unknown, null, or "".
I don't think we want a generic 'compare two values' function as we want the function to be able to provide feedback to the user via errors and warnings if no suitable value is found. A generic function cannot tell the user what value is missing.
This led me to thinking that we should implement logic specifically for retrieving the project value
This PR
In this PR I propose that we add a new embeddable struct that allows adding a new method GetProject to resources and data sources. This method will return either the resource's project argument, the provider-default project, or an error as feedback to the user.
The calling code that defines the custom model for that resource (e.g. GoogleFirebaseAppleAppConfigModel above) has the responsibility of needing to provide the correct field from the resource model struct as an argument. This replaces the use of "project" in the SDK version.
Because the method is specific to project, we can include feedback about which field has missing information.
An open question is whether we have any resources/data sources currently where a project ID is supplied by a field called something other than project. If not, we can hardcode the function to raise an error about project being unset. If there are special resources that use other fields for project we can update the logic so that the field name is stored in the resource model struct and accessed from there. However, setting the custom project schema field name in that struct is something that would need to happen in a non-standardised (see https://github.com/GoogleCloudPlatform/magic-modules/pull/11924) Configure function.
Release Note Template for Downstream PRs (will be copied)
Hi there, I'm the Modular magician. I've detected the following information about your changes:
Diff report
Your PR generated some diffs in downstreams - here they are.
google provider: Diff ( 23 files changed, 284 insertions(+), 236 deletions(-))
google-beta provider: Diff ( 29 files changed, 322 insertions(+), 333 deletions(-))
Errors
google-beta provider:
- The diff processor failed to build. This is usually due to the downstream provider failing to compile.
Tests analytics
Total tests: 0 Passed tests: 0 Skipped tests: 0 Affected tests: 0
Click here to see the affected service packages
All service packages are affected
Tests were added that are skipped in VCR:
- TestAccSdkProvider_impersonate_service_account_delegates 🔴 Errors occurred during REPLAYING mode. Please fix them to complete your PR.
View the build log
Hi there, I'm the Modular magician. I've detected the following information about your changes:
Diff report
Your PR generated some diffs in downstreams - here they are.
google provider: Diff ( 22 files changed, 283 insertions(+), 235 deletions(-))
google-beta provider: Diff ( 28 files changed, 327 insertions(+), 338 deletions(-))
Tests analytics
Total tests: 4174 Passed tests: 3756 Skipped tests: 413 Affected tests: 5
Click here to see the affected service packages
All service packages are affected
Action taken
Found 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
- TestAccDataSourceGoogleFirebaseAndroidAppConfig
- TestAccDataSourceGoogleFirebaseAppleAppConfig
- TestAccFirebaseWebApp_firebaseWebAppFull
- TestAccFrameworkProviderBasePath_setBasePath
- TestAccFrameworkProviderMeta_setModuleName
Get to know how VCR tests work
🟢 Tests passed during RECORDING mode:
TestAccDataSourceGoogleFirebaseAndroidAppConfig[Debug log]
TestAccDataSourceGoogleFirebaseAppleAppConfig[Debug log]
TestAccFirebaseWebApp_firebaseWebAppFull[Debug log]
TestAccFrameworkProviderBasePath_setBasePath[Debug log]
TestAccFrameworkProviderMeta_setModuleName[Debug log]
🟢 No issues found for passed tests after REPLAYING rerun.
🟢 All tests passed!
View the build log or the debug log for each test
Just rebased so that this PR only includes new commits, and none of the muxing fix commits
@c2thorn @NickElliot - I've added you as reviewers/assignees just so this doesn't fall off your radars, but I don't have any expectation of review. Feel free to implement these changes in different PRs if it's an approach that you want to use in future 😁
Hi there, I'm the Modular magician. I've detected the following information about your changes:
Diff report
Your PR generated some diffs in downstreams - here they are.
google provider: Diff ( 4 files changed, 119 insertions(+), 25 deletions(-))
google-beta provider: Diff ( 7 files changed, 143 insertions(+), 109 deletions(-))
Tests analytics
Total tests: 4311 Passed tests: 3901 Skipped tests: 407 Affected tests: 3
Click here to see the affected service packages
All service packages are affected
Action taken
Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
- TestAccApigeeDeveloper_apigeeDeveloperUpdateTest
- TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy
- TestAccContainerCluster_resourceManagerTags
Get to know how VCR tests work
🟢 Tests passed during RECORDING mode:
TestAccContainerCluster_resourceManagerTags [Debug log]
🟢 No issues found for passed tests after REPLAYING rerun.
🔴 Tests failed during RECORDING mode:
TestAccApigeeDeveloper_apigeeDeveloperUpdateTest [Error message] [Debug log]
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy [Error message] [Debug log]
🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.
View the build log or the debug log for each test
