refactor(gcp-link-account): Refactor GCP integration resource for readability #2785
Conversation
… to improve code readability
| } | ||
|
|
||
| func generateGcpIntegrationSchema() map[string]*schema.Schema { | ||
| baseSchema := cloudGcpIntegrationSchemaBase() |
There was a problem hiding this comment.
| baseSchema := cloudGcpIntegrationSchemaBase() | |
| gcp_integrations_base_schema := cloudGcpIntegrationSchemaBase() |
(just trying to correct the case, as generally used in Golang :) please rename this variable)
| s := cloudGcpIntegrationSchemaBase() | ||
| return &schema.Resource{ | ||
| Schema: s, | ||
| func addFetchTagsToSchema(baseSchema map[string]*schema.Schema) map[string]*schema.Schema { |
There was a problem hiding this comment.
@rpaliwal1997 amazing work on writing a common function containing the schema for most integrations :) this has helped us get rid of 200 lines of code.
I have a suggestion about this function. The purpose of this function is only to be called for the integrations which have an extra fetch_tags argument (alongside metrics_polling_interval), i.e. big_query, pub_sub, spanner and storage; so instead of having this function addFetchTagsToSchema, I would suggest using a similar approach using a helper function called mergeSchemas() we already have in the provider.
terraform-provider-newrelic/newrelic/helpers.go
Lines 125 to 133 in 25894e4
We already have a cloudGcpIntegrationsSchemaBase() which exports metrics_polling_interval; we can have a cloudGcpIntegrationsCustomSchema() which exports fetch_tags similar to cloudGcpIntegrationsSchemaBase(). Using a new function in this file (say cloudGcpIntegrationsAddCustomSchemaToBase()), we can use mergeSchemas() to combine schemas from both functions, and the output of this function can directly be referenced in the schema of pub_sub, spanner, storage, big_query. I wanted to suggest this as we can use this format to keep code consistent across the provider.
A very recent example of using this helper function is cloudAzureIntegrationMergeResourceGroupsElem() added/modified recently by Aashirwad. Search for this function on the main branch, and you'll get an idea of what I mean to say, as the usage of this function is pretty much similar.
| } | ||
| } | ||
| } | ||
|
|
||
| // flatten function to set(store) outputs from the terraform apply | ||
| func flattenCloudGcpAppEngineIntegration(in *cloud.CloudGcpAppengineIntegration) []interface{} { | ||
| func flattenCloudGcpCommonIntegration(in interface{}) []interface{} { |
There was a problem hiding this comment.
@rpaliwal1997 I have been deliberating/trying/experimenting to get rid of this function/simplifying the switch cases (while I fully know why you needed to add this :)) as we now have two functions instead of one with the exact same cases (of course, this is helping us remove so many other functions, but it doesn't really seem logical that we have two functions with the same cases).
However, from all of my experimentation in the last bunch of hours, I realize this is needed if we want to get rid of rest of the functions, because Go is such a rigid typed language; it all comes down to the fact that we cannot have a generic case written to match all of these types to get t.MetricsPollingInterval because all of these GcpCloudIntegration types are implementations of the CloudIntegrationInterface in Golang, but they don't have a custom implementation per class of GetMetricsPollingInterval to return the metrics polling interval (which would have been useful for our condition, but that's just how Tutone generates this from NerdGraph). Ideally the only other way to make this possible would be to write some custom interfaces of our own in the Go Client, but that is a bad solution.
In summary; I just wanted to share all this for general awareness :) since it looks like there's no other way, we can probably try to explore if there's still some other approach we're missing in the future, but for now, since the lint failure you're seeing on the PR is because of the cyclomatic complexity of this function, please add a // nolint:gocyclo comment on top of the function similar to what you will find above flattenCloudGcpLinkedAccount. We'll get the PR merged with this for now and then think of a probable, better solution.
|
@rpaliwal1997 I've asked for some changes in three comments; please take a look, fix them and then we should be good to go with a merge (but no release until the code freeze is done). Just make sure you get an extra pair of eyes before the merge 👍 |
Jira Ticket
Refactor GCP Integration Resource
Description: This pull request includes a refactor of the GCP integration resource in the resource_newrelic_cloud_gcp_integrations.go file to improve code readability and maintainability.
The changes include:
Refactored the resourceNewrelicCloudGcpIntegrations function to use a new generateGcpIntegrationSchema function for schema generation.
Reduced cyclomatic complexity in the flattenCloudGcpLinkedAccount function.
Changes:
Added generateGcpIntegrationSchema function to encapsulate schema generation logic.
Refactored flattenCloudGcpLinkedAccount to handle different GCP integrations more cleanly.
Improved code structure and organization for better readability.
Impact: These changes do not alter the functionality of the GCP integration resource but make the codebase easier to understand and maintain.
Type of change
Please delete options that are not relevant.
Checklist:
Please delete options that are not relevant.
How to test this change?
Please describe how to test your changes. Include any relevant steps in the UI, HCL file(s), commands, etc