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

Support http server by deferring to aws express lib. #584

Merged
merged 4 commits into from
Sep 9, 2018

Conversation

CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Sep 8, 2018

The azure-specific library we were using was having several problems that i found i couldn't effectively solve/workaround. For example, serving static files would just crash. The AWS library we are using works quite well on everything i've thrown at it so far. So, as a real solution, i just made our Azure impl sit on top of hte AWS library. This did require some marshalling to/from the azure and aws types. But this marshalling was trivial. On a good note, it should mean uniform behavior of http server on AWS/Azure, which is definitely desirable.

// the problem.
return context => {
context.log("Error occurred creating server: " + err.message + "\n" + err.stack);
context.done();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: these logs go into the Function's log stream (similar to cloudwatch). It is not returned to the caller (i.e. a 3rd part) of this http endpoint..

@lukehoban
Copy link
Member

Is it worth separating this out into it's own module (even just it's own file in this repo for now) that is independent of the rest of @pulumi/cloud, and which serves as a generic serverless-express layer that can work for AWS, Azure (and in the future GCP and others?).

I could imagine either upstreaming this or creating/sharing a serverless-express library as an independent thing in the future.

@CyrusNajmabadi
Copy link
Contributor Author

Is it worth separating this out into it's own module (even just it's own file in this repo for now) that is independent of the rest of @pulumi/cloud, and which serves as a generic serverless-express layer that can work for AWS, Azure (and in the future GCP and others?).

Yes. I think so. I'll open a bug to track that. That said, it's not high on my priority list. :)

@CyrusNajmabadi
Copy link
Contributor Author

@lukehoban Are you ok with this PR?

@@ -15,6 +15,8 @@
import * as pulumi from "@pulumi/pulumi";
import * as http from "http";

export type RequestListenerFactory = () => (req: http.IncomingMessage, res: http.ServerResponse) => void;
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a comment here, maybe even with a simple example implementation? This is really key to understanding the API here.

Copy link
Member

Choose a reason for hiding this comment

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

Does RequestListener come from Nodejs or Express api docs or dts files? Would love to align on naming if there is an established name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It comes from Nodejs. They call it "requestListener" as the argument to http.createServer. I'll add a link to the docs here.

headers: Record<string, string>;

// Currently unused. We may need to handle this to support binary data properly.
// isBase64Encoded?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to include this in the type, even if not yet used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No huge reason... I just felt it was good to call out that we hadn't explicitly added support for it.

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