This repository has been archived by the owner on Jun 11, 2019. It is now read-only.

Initial sketch for subscribing to azure functions off of triggers. #1

Merged

CyrusNajmabadi merged 20 commits into master from subscriptions

Aug 3, 2018

Contributor

CyrusNajmabadi commented Jun 28, 2018

No description provided.

Cyrus Najmabadi added 2 commits

June 27, 2018 20:07


          Initial subscription stubs.

9518a86


          Initial sketch for how subscriptions should work with aazure function…

7c19537

… apps.

Contributor Author

CyrusNajmabadi commented Jun 28, 2018

@lukehoban Take a look. Untested, but i think this should work properly. It basically takes Matt's work, but tries to library-fy it. It also tries to provide a pattern we can take for all the rest of the serverless azure stuff. I'll comment the particular points i thought were salient in this PR.

Note: i tried to keep this similar to aws-serverless, just matching more of the azure concepts where appropriate.

CyrusNajmabadi commented

View reviewed changes

nodejs/azure-serverless/blob.ts Outdated

		@@ -0,0 +1,156 @@
		// Copyright 2016-2018, Pulumi Corporation.

Contributor Author

CyrusNajmabadi Jun 28, 2018

Start with subscription.ts first.

CyrusNajmabadi commented

View reviewed changes

nodejs/azure-serverless/subscription.ts

+               * return nothing, and should signal that it is done by calling `context.Done()`. Errors can be
+               * signified by calling `context.Done(err)`
+               */
+              export type Callback<C extends Context, Data> = (context: C, data: Data) => void;

Contributor Author

CyrusNajmabadi Jun 28, 2018

Unfortunately:

this can't have a Promise return type afaict (unless azure has added support for that). We could consider providing separate entrypoints to allow someone to pass an async callback. We'd then ".then" that, calling context.Done(...).
As we talked about, this can't be a callback or lambda. We can't make a trigger with a preexisting function. So we only support the callback form here.

CyrusNajmabadi commented

View reviewed changes

nodejs/azure-serverless/subscription.ts

+               */
+              export type Callback<C extends Context, Data> = (context: C, data: Data) => void;
+              export interface EventSubscriptionArgs {

Contributor Author

CyrusNajmabadi Jun 28, 2018

We talkd today about being opinionated here. But it turned out to just be trivial to let the caller supply these sorts of values if they did want to control the FunctionApp they were creating. I figured it was reaosnable to just have this. As you can see, it's all optional. So there is an easy path to make FunctionApps. But you can provide these values if you want.

CyrusNajmabadi commented

View reviewed changes

nodejs/azure-serverless/subscription.ts Outdated

+                      });
+              }
+              function signedBlobReadUrl(

Contributor Author

CyrusNajmabadi Jun 28, 2018

Copied directly from the example. I assume this works...

CyrusNajmabadi commented

View reviewed changes

nodejs/azure-serverless/subscription.ts Outdated

+              import * as azurestorage from "azure-storage";
+              const config = new pulumi.Config("azure");
+              const region = config.require("region");

Contributor Author

CyrusNajmabadi Jun 28, 2018

opinionated way to specify the region.


          Formatting.

CyrusNajmabadi commented

View reviewed changes

nodejs/azure-serverless/subscription.ts

+                  readonly resourceGroup: azure.core.ResourceGroup;
+                  readonly storageAccount: azure.storage.Account;
+                  readonly storageContainer: azure.storage.Container;
+                  readonly appServicePlan: azure.appservice.Plan;

Contributor Author

CyrusNajmabadi Jun 28, 2018

these four values are the ones we may end up creating ourself. So i thought it was sensible to expose off of the subscription.


          Add comment

c9e2ca7

CyrusNajmabadi commented

View reviewed changes

nodejs/azure-serverless/subscription.ts Outdated

+                          storageConnectionString: this.storageAccount.primaryConnectionString,
+                          appSettings: {
+                              "WEBSITE_RUN_FROM_ZIP": codeBlobUrl,

Contributor Author

CyrusNajmabadi Jun 28, 2018

later you'll see the storage connection string injected directly into the Binding. I think the azury way to do things is to put a setting here and map it to the connection string. then the binding just references the appsetting key.

I could make that work. it just makes things a bit more complex as the caller has to pass along those extra appsettings. But if you think i should do this, i can!

CyrusNajmabadi commented

View reviewed changes

nodejs/azure-serverless/blob.ts Outdated


		import * as subscription from "./subscription";

		interface BlobBinding extends subscription.Binding {

Contributor Author

CyrusNajmabadi Jun 28, 2018

note: this is an internal type. so it exists mostly just to document certain properties and to make it clear that the values we're passing are intentional.

CyrusNajmabadi commented

View reviewed changes

nodejs/azure-serverless/blob.ts Outdated

+                      "uri": string;
+                      "properties": {
+                          "cacheControl": any,
+                          "contentDisposition": any,

Contributor Author

CyrusNajmabadi Jun 28, 2018

things that are 'any' in this list basically got passed to me as 'null', so i didn't know how to determine what the type actually is easily. effectively, i'm lazy and didn't want to go hunt these down (yet).

Member

lukehoban commented Jul 2, 2018 •

edited

Loading

I've been trying this out, and hit a few issues. Curious - was this working as-is for you?

Summarizing a few of the things I hit:

Can't pass Output<Archive> to ZipBlob. Appears to be a bug in pulumi-terraform. Cannot pass Output<Archive> when Input<Archive> is specified pulumi-terraform#191
Can't pass connection string as connection directly, must embed in a separate AppSettings variable and reference from there by name? ServiceBusQueueTrigger not working with azure-functions-cli Azure/azure-functions-core-tools#128
Should use Account instead of Blob as the scope for operations. (Or Container, but I think Account with filters that specify path through containers will be easier and more aligned with Azure, as it seems less common to have a first-class handle on the Container object).
Would love an example
There is use of a region config variable, but that is a concept we don't really have in Azure. If we need/want to introduce something global for location and/or resourceGroup, we should think about how to do that more generally.
We probably want/need to enable AppInsights for Functions by default (or with config)? Seems that is the normal way to get high level logs/metrics for functions, and the default when creating Functions through portal?

Would be great to have something working E2E with an example so we can discuss the API design and some of the cross-cutting parameterization questions relative to that.

lukehoban and others added 8 commits

July 2, 2018 10:32


          Blob events working

182a95f


          Invokeing container instance from handler

92132ea


          Clean up example

80cfcbb


          Simplify SAS URL computation and avoid duplication

36a805a

We can use the built-in `azure.storage.getAccountSAS` to compute SAS URLs instead of making calls out to the Azure SDK at deployment time.


          Clean things up wrt appsettings.

c4b27ad


          Move code.

3fbb773


          Tweaks.

d290b77


          Cleanup examples.

42cd077

Member

lukehoban commented Aug 1, 2018 •

edited

Loading

We merged #2 which built on top of these same commits - so I assume we can close this one out now?

[EDIT] I see that #2 was actually a PR into this PR! So we haven't merged the functionality here yet.

lukehoban mentioned this pull request

Create an azure-serverless package pulumi/pulumi-azure#61

Closed

Contributor Author

CyrusNajmabadi commented Aug 2, 2018

[EDIT] I see that #2 was actually a PR into this PR! So we haven't merged the functionality here yet.

Indeed :)

Cyrus Najmabadi and others added 3 commits

August 3, 2018 08:38


          Test stub.

9ea408f


          Whitespace.

1a49488


          Sync travis.yml

78a022b

Contributor Author

CyrusNajmabadi commented Aug 3, 2018

@ellismg Can you help here. It looks like something isn't setup properly with travis here:

$ git clone git@github.com:pulumi/scripts ${GOPATH}/src/github.com/pulumi/scripts
Cloning into '/home/travis/gopath/src/github.com/pulumi/scripts'...
Warning: Permanently added the RSA host key for IP address '192.30.253.112' to the list of known hosts.
Permission denied (publickey).
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
The command "git clone git@github.com:pulumi/scripts ${GOPATH}/src/github.com/pulumi/scripts" failed and exited with 128 during .


          Add test.

CyrusNajmabadi force-pushed the subscriptions branch from 52c7f6d to 1a49488 Compare

August 3, 2018 16:40

Cyrus Najmabadi added 4 commits

August 3, 2018 09:48


          Add tests.

50e2bbf


          Fix test file.

6e29b8c


          Strip out packages.

ae48fc9


          Pick a fixed start date for signatures.

c94f276

CyrusNajmabadi merged commit 64b0a87 into master

pulumi-bot deleted the subscriptions branch

August 3, 2018 19:43

CyrusNajmabadi mentioned this pull request

Create an equivalent of serverless.Function pulumi/pulumi-azure#60

Closed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet