-
Notifications
You must be signed in to change notification settings - Fork 118
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
Migrate from SDKv2 => Framework #142
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.
This is looking extremely good already. Just left some advices around declaring explicitly the interfaces you intend to implement, so the compiler can help you down the line.
"github.com/hashicorp/terraform-plugin-framework/tfsdk" | ||
"github.com/hashicorp/terraform-plugin-framework/types" | ||
) | ||
|
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.
One thing that I picked up is to make sure I declare top of the file what interfaces each type is expected to implement.
For example, in the TLS FW PR I have this pretty much everywhere:
type (
privateKeyResourceType struct{}
privateKeyResource struct{}
)
var (
_ tfsdk.ResourceType = (*privateKeyResourceType)(nil)
_ tfsdk.Resource = (*privateKeyResource)(nil)
)
This ensures that a) all the interfaces are enforced at compile time b) notice that I'm forcing receivers to be pointers, to avoid any surprise down the line where a call to a mutating method, doesn't apply the change to the struct in the expected way.
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.
Good shout. Have added in the relevant interface implementations.
internal/provider/provider.go
Outdated
func New() *schema.Provider { | ||
return &schema.Provider{ | ||
Schema: map[string]*schema.Schema{}, | ||
type provider struct { |
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.
Same as above here, about enforcing interfaces at compile time.
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.
Fixed.
internal/provider/provider_test.go
Outdated
"http": func() (tfprotov6.ProviderServer, error) { | ||
return providerserver.NewProtocol6(New())(), nil | ||
}, |
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.
Drive-by question: Can this use providerserver.NewProtocol6WithError(New())
instead?
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.
Sure thing! Done.
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 good to me 🚀 Excellent work
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR contains breaking changes for:
Closes Allow accepting additional StatusCodes with custom fallback response #114
Closes: #140
Closes: #114
Closes: #107
Closes: #93
Closes: #41