-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to use Terraform Multiplexer #565
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff! Just a few minor nits regarding naming/style and duplicated code
@@ -9,7 +9,7 @@ import ( | |||
"io" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can deduplicate this code by writing a method that converts the diagnostics. I would use this file as the default implementation, and have the conversion method take new diags, and convert them to old diags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretically, this should be the statuspage
that we end up using in the long run anyway. And the old one should go away. I think the duplication makes sense when all of the products convert and we can remove the old code. And I don't know if we gain a lot by having a method to convert diagnostics, if this is one of the only methods we have to do it for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will take a long time to convert all resources, so I would prioritize minimal duplication where possible. Converting diagnostics should be trivial, there may even be a method for it already in the mux package somewhere.
} | ||
|
||
// getProjectFromCredentials uses the configured client credentials to | ||
// fetch the associated organization and returns that organization's | ||
// single project. | ||
func getProjectFromCredentials(ctx context.Context, client *clients.Client) (project *models.HashicorpCloudResourcemanagerProject, diags diag.Diagnostics) { | ||
// This differs from the provider.go implementation due to the diagnostics used | ||
// by the plugin framework. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should deduplicate this code, similar to the approach described for statuspage.go
} | ||
|
||
// getOldestProject retrieves the oldest project from a list based on its created_at time. | ||
func GetOldestProject(projects []*models.HashicorpCloudResourcemanagerProject) (oldestProj *models.HashicorpCloudResourcemanagerProject) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is duplicated 1:1 in the framework provider. Can this be deduped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to unblock. We can follow up on some of the duplicated code in a future PR
🛠️ Description
This PR introduces the Terraform framework multiplexer at the top level to allow for the migration of Terraform resources from the SDKv2 to the plugin framework.
This also includes migrating the Vault Secrets App data source to the new plugin framework.
There have been manual upgrade tests run on the following resources by creating resources using the HCP Provider version
0.68.0
and switching to a local dev provider with the muxer changes to verify no changes were expected. The following manual tests were run on the following resources:This also contains maintainability fixes moving forward:
providersdkv2
🏗️ Acceptance tests
Output from acceptance testing: