Skip to content
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

Ensure forward build compatibility for resource provider #675

Merged
merged 2 commits into from Dec 11, 2022

Conversation

AaronFriel
Copy link
Member

@github-actions
Copy link

Diff for pulumi-random with merge commit 553300c

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 553300c

@github-actions
Copy link

Diff for pulumi-random with merge commit d5bc837

@github-actions
Copy link

Diff for pulumi-azuread with merge commit d5bc837

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 553300c

@github-actions
Copy link

Diff for pulumi-gcp with merge commit d5bc837

@github-actions
Copy link

Diff for pulumi-aws with merge commit 553300c

@github-actions
Copy link

Diff for pulumi-aws with merge commit d5bc837

@AaronFriel AaronFriel added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Dec 10, 2022
@@ -32,6 +33,8 @@ import (
var _ = (plugin.Provider)((*inmemoryProvider)(nil))

type inmemoryProvider struct {
pulumirpc.UnimplementedResourceProviderServer
Copy link
Member

Choose a reason for hiding this comment

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

Could clean up all the panic unimplemented methods in this file

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it but I wondered if it would change the behavior in any way. The unimplemented server returns a grpc error rather than panic, yeah?

Does seem unlikely anything depends on these methods panicking.

Copy link
Member

Choose a reason for hiding this comment

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

Yeh I think it was just an easier way to write the method, its used in a part of code that doesn't call any of this stuff anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, the embedding here was incorrect anyway. I've added a PR to pu/pu to add an embeddable plugin.UnimplementedProvider: pulumi/pulumi#11618

Copy link
Member Author

Choose a reason for hiding this comment

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

@Frassle updated this targeting latest Pulumi, embedded plugin.UnimplementedProvider and deleted the panicking methods.

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 5b769e2

@github-actions
Copy link

Diff for pulumi-random with merge commit 5b769e2

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 5b769e2

@github-actions
Copy link

Diff for pulumi-aws with merge commit 5b769e2

bors bot added a commit to pulumi/pulumi that referenced this pull request Dec 10, 2022
11618: Add forward compatible plugin.UnimplementedProvider r=AaronFriel a=AaronFriel

This adds an implementation of Provider that can be embedded for forward compatibility.

A question was raised in pulumi/pulumi-terraform-bridge#675 about why the PR did not delete the default panicking implementations of various methods. In doing so, I discovered the in memory provider implementation used the Pulumi SDK's `Provider` interface, not the usual gRPC server interface.

With this change, we'll be able to amend the PR to the bridge to embed a long-term compatible interface.

Co-authored-by: Aaron Friel <mayreply@aaronfriel.com>
@github-actions
Copy link

Diff for pulumi-random with merge commit 647f0fe

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 647f0fe

@github-actions
Copy link

Diff for pulumi-random with merge commit bcbc456

@github-actions
Copy link

Diff for pulumi-azuread with merge commit bcbc456

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 647f0fe

@github-actions
Copy link

Diff for pulumi-gcp with merge commit bcbc456

@github-actions
Copy link

Diff for pulumi-aws with merge commit 647f0fe

@github-actions
Copy link

Diff for pulumi-aws with merge commit bcbc456

@AaronFriel AaronFriel merged commit b1a4e2c into master Dec 11, 2022
@AaronFriel AaronFriel deleted the friel/fwd-compat branch December 11, 2022 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants