chore: Adds provider_meta support to the provider #3618
Conversation
…r agent operations
There was a problem hiding this comment.
Pull Request Overview
This PR adds provider_meta support to enable module-level metadata tracking in the MongoDB Atlas Terraform provider. The feature allows terraform configurations to specify module names, versions, and additional User-Agent data that gets included in HTTP requests for analytics purposes.
- Implements provider_meta schema support for both framework and SDK v2 providers
- Adds User-Agent enhancement system to capture and transmit module metadata in HTTP headers
- Creates wrapper resources that inject analytics data into terraform operations
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/provider/provider.go | Adds MetaSchema method and wraps resources with analytics functionality |
| internal/provider/provider_sdk2.go | Implements ProviderMetaSchema for SDK v2 and wraps resources |
| internal/config/user_agent.go | Core User-Agent metadata handling and context management |
| internal/config/user_agent_test.go | Comprehensive test coverage for User-Agent functionality |
| internal/config/transport.go | HTTP transport wrapper for User-Agent header modification |
| internal/config/resource_base.go | Framework resource wrapper with analytics integration |
| internal/config/resource_base_sdkv2.go | SDK v2 resource wrapper with analytics integration |
| internal/config/resource_base_test.go | Tests ensuring resource interface preservation |
| internal/config/transport_test.go | Minor formatting cleanup |
| internal/config/client.go | Updates HTTP client transport chain configuration |
| internal/testutil/unit/provider_mock.go | Simplifies mock provider by using embedding |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
|
This PR has gone 7 days without any activity and meets the project’s definition of "stale". This will be auto-closed if there is no new activity over the next 7 days. If the issue is still relevant and active, you can simply comment with a "bump" to keep it open, or add the label "not_stale". Thanks for keeping our repository healthy! |
| type RSCommon struct { | ||
| ImplementedResource | ||
| Client *MongoDBClient | ||
| ResourceName string |
There was a problem hiding this comment.
if we have now GetName, do we need uppercase ResourceName or could be private and use GetName when needed? similarly for Client, if we add GetClient (apart from SetClient)
There was a problem hiding this comment.
Want to avoid having to change every single resource 😅
There was a problem hiding this comment.
fair enough, also it can be done later
| func analyticsResource(iResource ImplementedResource) resource.Resource { | ||
| return &RSCommon{ | ||
| ResourceName: iResource.GetName(), | ||
| ImplementedResource: iResource, | ||
| } | ||
| } |
There was a problem hiding this comment.
| func analyticsResource(iResource ImplementedResource) resource.Resource { | |
| return &RSCommon{ | |
| ResourceName: iResource.GetName(), | |
| ImplementedResource: iResource, | |
| } | |
| } | |
| func analyticsResource(iResource ImplementedResource) resource.Resource { | |
| return iResource | |
| } |
Might be missing something but I believe this function is redundant, ImplementedResource already has resource.ResourceWithImportState interface which is a resource.Resource.
There was a problem hiding this comment.
Good catch, but the function is actually necessary! While ImplementedResource does implement resource.Resource, we need to wrap it with RSCommon to intercept CRUD operations.
If we returned iResource directly, the analytics tracking wouldn't work - RSCommon is what adds the provider_meta information to the context before calling the underlying resource methods. Without the wrapper:
Create/Read/Update/Deletewould execute directly on the resource without adding analytics to context- The
UserAgentTransportwould receive empty context and no analytics would be tracked
Added text in d5452ab
There was a problem hiding this comment.
I see. I am mainly confused because all our resource implementations are already defined with RSCommon (example), so we are going from resource implementation which is RSCommon returned as resource.Resource, which then in AnalyticsResourceFunc gets wrapped in RSCommon again.
There was a problem hiding this comment.
Yes, it is a bit confusing but I don't know a better way.
The alternative would be to pass the CRUD operations to the RSCommon and then have that wrap those methods. But that requires changing every single resource.
This could work a bit safer if new methods are added
There was a problem hiding this comment.
I'm trying to see how we could change each resource:
func Resource() resource.Resource {
return &rs{
RSCommon: config.RSCommon{
ResourceName: resourceName,
},
}
}But I'm not sure how to do this in a "clean" way.
What I like about the current implementation is that it keeps the existing resources untouched.
So whenever you add a resource you don't need to worry about the analytics part unless you use a new TPF method that hasn't been wrapped.
| We are not initializing deprecated fields, for example Update to avoid the message: | ||
| resource mongodbatlas_cloud_backup_snapshot: All fields are ForceNew or Computed w/out Optional, Update is superfluous | ||
| Ensure no deprecated fields are used by running `staticcheck ./internal/service/... | grep -v 'd.GetOkExists'` and looking for (SA1019) |
There was a problem hiding this comment.
Would this be something to take in considerations when upgrading terraform-plugin-sdk?
There was a problem hiding this comment.
Don't think it is different than the resources we already have.
But it wouldn't hurt to start running the staticcheck command mentioned in the comment to catch usage of deprecated fields but outside the scope of this PR
- Explain why analyticsResource wrapper is necessary for intercepting CRUD ops - Document complete UserAgentExtra context flow from provider_meta to HTTP transport
AgustinBettati
left a comment
AgustinBettati
left a comment
There was a problem hiding this comment.
LGTM, we can continue conversation in #3618 (comment) in case there is room for reducing complexity but not a blocker
Adds provider_meta support to all data sources (TPF and SDKv2) for module usage tracking via User-Agent headers. Follows the same pattern as resources (PR #3618). - Created datasource_base.go for data source analytics in TPF - Added SetClient() method pattern matching resources - Shared asUserAgentExtraFromProviderMeta() between resources and data sources
Description
Adds provider_meta support to the provider
Follow up PR for data sources
Link to any related issue(s): CLOUDP-340210
Example usage
Terraform code
main.tfTerraform code modules/project/main.tf
Logs example
terraform plan(seeName/projectandName/custom_db_role)Logs example
terraform applyType of change:
Required Checklist:
Further comments