Skip to content

Use proto v5 mux server #215

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

Merged
merged 3 commits into from
Dec 5, 2022
Merged

Use proto v5 mux server #215

merged 3 commits into from
Dec 5, 2022

Conversation

k-yomo
Copy link
Contributor

@k-yomo k-yomo commented Dec 4, 2022

#214

This PR adds mux server in reference to aws provider.
https://github.com./hashicorp/terraform-provider-aws/blob/main/internal/provider/factory.go

With this change, we'll be able to use terraform-plugin-framework's provider while keeping existing provider like below.
https://github.com./hashicorp/terraform-provider-aws/blob/d82c79b1e39200aeee5f7928f015af00cfcc633f/internal/provider/factory.go#L23-L26C3

This change won't affect to the users of the provider.

@k-yomo k-yomo changed the title Use proto v5 server Use proto v5 mux server Dec 4, 2022
@k-yomo k-yomo force-pushed the use-proto-v5-server branch from af144f7 to 9831ce8 Compare December 4, 2022 05:57
@k-yomo k-yomo marked this pull request as ready for review December 4, 2022 06:08
Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor nit on the variable naming.

// ProtoV5ProviderServerFactory returns a muxed terraform-plugin-go protocol v5 provider factory function.
// The primary (Plugin SDK) provider server is also returned (useful for testing).
func ProtoV5ProviderServerFactory(ctx context.Context, version string) (func() tfprotov5.ProviderServer, *schema.Provider, error) {
primary := New(version)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

primary doesn't seem like the right name moving forward. Once we have a second provider based on the plugin framework then this would at best be secondary.

Perhaps something like sdk2Provider or something similar instead?

"github.com./hashicorp/terraform-plugin-sdk/v2/helper/schema"
)

var Providers map[string]func() (*schema.Provider, error)
var Providers map[string]func() (tfprotov5.ProviderServer, error)
var Provider *schema.Provider
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is almost always just used to grab an Elasticsearch client and should be removed IMO. Calling it out here since it removes the need to return the primaryProvider from the server factory.

return nil, nil, err
}

return muxServer.ProviderServer, primary, nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, returning the primary provider makes sense right now, but would start to become more confusing once the migration was actually underway.

Happy to ignore this one for now, and remove the acctest.Provider variable in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true! Let me make another PR to remove the acctest.Provider!

@k-yomo k-yomo requested a review from tobio December 5, 2022 02:39
@tobio tobio merged commit ae3fd30 into elastic:main Dec 5, 2022
@k-yomo k-yomo deleted the use-proto-v5-server branch December 5, 2022 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants