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
[sdk/nodejs,python] Add support for explicit providers for packaged components #13282
Conversation
Changelog[uncommitted] (2023-07-13)Features
|
Hey @justinvp what's the latest on this? Are we just waiting on a review? Thanks! |
c14ed54
to
60f3da9
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.
Code and tests look good to me ✅
…onents Add integration tests for passing an explicit provider for a packaged component. The Go test already passes. The Node and Python tests fail.
Update the test for the behavior we expect. Currently fails.
Updates the Node.js SDK to support passing an explicit provider.
Currently fails without the fix.
Updates the Python SDK to support passing an explicit provider.
60f3da9
to
8f951da
Compare
bors merge |
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
The `Resource` class in the Node.js SDK has the following internal property: ```typescript /** @internal */ readonly __prov?: ProviderResource; ``` When a resource is created, the provider specified for the resource is stored in this property. If it is set, it is passed along in the `Call` request when a method is called on the resource. Prior to #13282, the property was only set for custom resources in `Resource`'s constructor: ```typescript this.__prov = custom ? opts.provider : undefined; ``` With #13282, it was changed to also store the value for remote components: ```diff - this.__prov = custom ? opts.provider : undefined; + this.__prov = custom || remote ? opts.provider : undefined; ``` This regressed the behavior when calling a method on a remote component that had an explicit provider that wasn't the component provider, but some other provider (e.g. AWS provider) specified as: ```typescript const component = new MyRemoteComponent("comp", { }, { provider: awsProvider }); ``` The `awsProvider` was being stored in `Resource.__prov`, and when making the method call on the resource, it would try to invoke `Call` on the AWS provider, rather than calling the remote component provider's `Call`, which resulted in an error. Note that specifying the AWS provider using the more verbose `providers: [awsProvider]` works around the issue. The fix is to only set `__prov` if the provider's package is the same as the resource's package. Otherwise, don't set it, because the user is specifying a provider with the `provider: awsProvider` syntax as shorthand for `providers: [awsProvider]`.
The `Resource` class in the Node.js SDK has the following internal property: ```typescript /** @internal */ readonly __prov?: ProviderResource; ``` When a resource is created, the provider specified for the resource is stored in this property. If it is set, it is passed along in the `Call` request when a method is called on the resource. Prior to #13282, the property was only set for custom resources in `Resource`'s constructor: ```typescript this.__prov = custom ? opts.provider : undefined; ``` With #13282, it was changed to also store the value for remote components: ```diff - this.__prov = custom ? opts.provider : undefined; + this.__prov = custom || remote ? opts.provider : undefined; ``` This regressed the behavior when calling a method on a remote component that had an explicit provider that wasn't the component provider, but some other provider (e.g. AWS provider) specified as: ```typescript const component = new MyRemoteComponent("comp", { }, { provider: awsProvider }); ``` The `awsProvider` was being stored in `Resource.__prov`, and when making the method call on the resource, it would try to invoke `Call` on the AWS provider, rather than calling the remote component provider's `Call`, which resulted in an error. Note that specifying the AWS provider using the more verbose `providers: [awsProvider]` works around the issue. The fix is to only set `__prov` if the provider's package is the same as the resource's package. Otherwise, don't set it, because the user is specifying a provider with the `provider: awsProvider` syntax as shorthand for `providers: [awsProvider]`.
The `Resource` class in the Node.js SDK has the following internal property: ```typescript /** @internal */ readonly __prov?: ProviderResource; ``` When a resource is created, the provider specified for the resource is stored in this property. If it is set, it is passed along in the `Call` request when a method is called on the resource. Prior to #13282, the property was only set for custom resources in `Resource`'s constructor: ```typescript this.__prov = custom ? opts.provider : undefined; ``` With #13282, it was changed to also store the value for remote components: ```diff - this.__prov = custom ? opts.provider : undefined; + this.__prov = custom || remote ? opts.provider : undefined; ``` This regressed the behavior when calling a method on a remote component that had an explicit provider that wasn't the component provider, but some other provider (e.g. AWS provider) specified as: ```typescript const component = new MyRemoteComponent("comp", { }, { provider: awsProvider }); ``` The `awsProvider` was being stored in `Resource.__prov`, and when making the method call on the resource, it would try to invoke `Call` on the AWS provider, rather than calling the remote component provider's `Call`, which resulted in an error. Note that specifying the AWS provider using the more verbose `providers: [awsProvider]` works around the issue. The fix is to only set `__prov` if the provider's package is the same as the resource's package. Otherwise, don't set it, because the user is specifying a provider with the `provider: awsProvider` syntax as shorthand for `providers: [awsProvider]`.
The `Resource` class in the Node.js SDK has the following internal property: ```typescript /** @internal */ readonly __prov?: ProviderResource; ``` When a resource is created, the provider specified for the resource is stored in this property. If it is set, it is passed along in the `Call` request when a method is called on the resource. Prior to #13282, the property was only set for custom resources in `Resource`'s constructor: ```typescript this.__prov = custom ? opts.provider : undefined; ``` With #13282, it was changed to also store the value for remote components: ```diff - this.__prov = custom ? opts.provider : undefined; + this.__prov = custom || remote ? opts.provider : undefined; ``` This regressed the behavior when calling a method on a remote component that had an explicit provider that wasn't the component provider, but some other provider (e.g. AWS provider) specified as: ```typescript const component = new MyRemoteComponent("comp", { }, { provider: awsProvider }); ``` The `awsProvider` was being stored in `Resource.__prov`, and when making the method call on the resource, it would try to invoke `Call` on the AWS provider, rather than calling the remote component provider's `Call`, which resulted in an error. Note that specifying the AWS provider using the more verbose `providers: [awsProvider]` works around the issue. The fix is to only set `__prov` if the provider's package is the same as the resource's package. Otherwise, don't set it, because the user is specifying a provider with the `provider: awsProvider` syntax as shorthand for `providers: [awsProvider]`. Fixes #13777
Add support for explicit providers for packaged components in the Node.js and Python SDKs.
Go already supports it. Node.js briefly supported it with #11093, but the change was reverted in #11509 due to an issue. Python has never supported it.
The PR is broken up into multiple commits for easier reviewing.
Fixes #13074
Part of #11520