-
Notifications
You must be signed in to change notification settings - Fork 150
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
Provide a way to expose the real aws-sdk simply through the pulumi API at runtime #365
Conversation
88a1b5b
to
3dc2e3b
Compare
Working on a PR showing how this looks to consume downstream. |
|
@@ -12,9 +12,12 @@ | |||
// See the License for the specific language governing permissions and | |||
// limitations under the License. | |||
|
|||
// Re-export the awssdk.DynamoDB class/namespace out as 'runtime' so there is easy access to this | |||
// API at actual cloud-runtime. | |||
export { DynamoDB as runtime } from "aws-sdk"; |
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.
note: this gives us a reasonable shaped API that a downstream consumer can use. However, there are severe closure-serialization complications in play (see main comments), which would have to be addressed before this coudl actually realistically be consumed.
@@ -2037,6 +2037,7 @@ func Provider() tfbridge.ProviderInfo { | |||
JavaScript: &tfbridge.JavaScriptInfo{ | |||
Dependencies: map[string]string{ | |||
"@pulumi/pulumi": "dev", | |||
"aws-sdk": "^2.0.0", |
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.
Note: this means that if a person requests a different sdk version, they'll get wrong TS typings. However, that's a known limitation of the awssdk. They even call it out here: https://github.com/aws/aws-sdk-js#known-limitations
There are a few known limitations with the bundled TypeScript definitions at this time:
Service client typings reflect the latest apiVersion, regardless of which apiVersion is specified when creating a client.
}) | ||
Object.defineProperty(pulumiSns, "runtime", { | ||
get: () => require("aws-sdk").SNS, | ||
}) |
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.
Note: i put everything in a single file for the following reasons:
- it gave us a centralized location to update whenever we need to make a change.
- it's actually nigh-impossible to expose these 'runtime' properties from teh sub-module themselves. The reason for this is due to how typescript exports get rewritten. Specifically, ts will rewrite exports as:
function __export(m) {
for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
}
// Export members:
__export(require("./dynamodbMixins"));
So even if we tried to do the trick where the property is a 'getter', during re-export the getter will be called and thus we would end up trying to capture the awssdk submodule itself, instead of caturing the getter.
@lukehoban Can you takea look when you get a chance. This approach works, but i'm curious how you feel about it. You can see how it can be used downstream with: pulumi/pulumi-cloud#645 |
sdk/nodejs/awsMixins.ts
Outdated
|
||
declare module "./sns" { | ||
export const runtime: awsSdkType["SNS"]; | ||
} |
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.
another option would be to just expose 'runtime' off of the root pulumi/aws module. 'runtime' would simply give you back require("aws-sdk")
. So on the consumption side in pulumi cloud you'd say something like new aws.runtime.DynamoDB.DocumentClient()
instead of new aws.dynamodb.runtime.DocumentClient()
. I tend to actually think the former is a bit nicer as it's just a single entrypoint into the aws-sdk, and hten you have the entire normal surface area available to you. Thoughts @lukehoban ?
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.
I actually went with this path.
I'm going to move forward with this. This is small enough that if we don't want this we can easily roll things back. |
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.
Thought I had hit send on this yesterday - but looks like I never submitted the review.
I like going with this simpler approach for now. But I do wonder if sdk
is a better name given what common usage looks like.
* | ||
* Note: this property will give you the aws-sdk module that AWS automatically includes | ||
* with any javascript Lambda. | ||
*/ | ||
export const runtime: typeof import("aws-sdk"); |
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.
Should we call it aws.sdk
since it gets an instance of aws-sdk
? It reads a little funny in usage to call this runtime
.
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.
i definitely thought about that. But found it a bit strange to use the term 'sdk' inside an 'sdk' :) But if you do like that better, i think i'm totally ok with it. It also doesn't really need to be called 'runtime' since it is totally usable at deployment time as well.
Ready to review. This allows clients in downstream libraries to write the following in their lambdas: