-
Notifications
You must be signed in to change notification settings - Fork 7
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 terraform-plugin-framework. #136
Update to terraform-plugin-framework. #136
Conversation
4d848f3
to
2053860
Compare
Thank you so much for your contributions @asaba-hashi ! I'm starting to review the PR now. Do you know if there are breaking changes to the interface/usage of the provider. I'm thinking the next release will be v1.0.0, so it'll be okay either way. |
There are some inconsistencies in how the JSON strings and line breaks are normalized in the state that will need to be updated for an improved experience in future PRs. What is implemented in this PR matches current behavior captured by the test cassettes. |
@zemberdotnet Pushed a commit that should fix the GitHub actions to use the correct version of go. Also opened #137 as a known issue that exists with the current release version that will also need to be addressed in a future PR. |
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.
Thanks again for your contributions @asaba-hashi. I've reviewed with mostly questions. I'm not the most familiar with the new plugin framework or providers in general, so pardon my ignorance if any of the questions seem out of place.
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.
@zemberdotnet I have pushed a couple updates for some changes to the PR, but I think as a path forward, I would like to get this PR merged with the smallest (if any) breaking changes, then addressing #138 and #137, and any others that make sense for breaking changes before another release, WDYT?
The version of |
Thanks @asaba-hashi . I'm running the provider locally and just checking everything works with the live API. Will report a few small changes soon. |
@asaba-hashi I opened a PR in your fork with my suggested changes. I got a little lost adding the logic for fetching the |
13edf31
to
749bf5b
Compare
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.
Looks great! @asaba-hashi
This PR updates the provider to use the
terraform-plugin-framework
rather than the SDK for some the key benefits listed here. The highest impact one here is simply reading the config directly into structs that more closely resemble the API objects given the high number of fields requiring configuration.While new resources could be added using the framework using the muxing support, with just 2 resources implemented, I think it makes sense to just do the migration instead of adding the complexity of multiple underlying SDKs.
The test cases have been minimally changed to check against the error strings of the new validators and updated for the new configuration.
One major challenge for adapting to the framework APIs was injecting the
go-vcr
enabledhttp.Client
into the provider. This is difficult to configure as the framework uses factory patterns to create and destroy provider instances. This is currently implemented by setting an optional, private fieldhttpClient
in the provider that is used in the test cases.