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

Get Azure Blobs example working #2

Merged
merged 4 commits into from
Jul 25, 2018
Merged

Conversation

lukehoban
Copy link
Member

This builds on the subscription branch to finish getting Blobs working end-to-end. Opening as a PR into that branch so that @CyrusNajmabadi can take a look and integrate if needed with that existing PR.

There are a few lingering issues:

  • The workaround for Cannot pass Output<Archive> when Input<Archive> is specified pulumi-terraform#191 causes initial preview to fail, to initially stand up the example a pulumi update --skip-preview is required.
  • The mapping of connection string into app settings + bindings.json is a little hacky - ideally the BlobBinding datastructure would be the real schema for the bindings instead of having to rewrite it as this code does.
  • The hard-coding of West US is bad. I actually think we should get rid of the "automatically create a ResourceGroup" code paths here, and make users pass resource group down through all abstractions for now. It's ugly, but consistent with all other Azure APIs. If we're going to solve for that - we should likely do it in some deeper way.

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.
const connectionString = (bindings[0] as any).connection;
const appSettings: Record<string, string> = {};
if (connectionString) {
(bindings[0] as any).connection = "FOO";
Copy link
Contributor

Choose a reason for hiding this comment

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

ick? what for.

Copy link
Member Author

Choose a reason for hiding this comment

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

See PR summary:

The mapping of connection string into app settings + bindings.json is a little hacky - ideally the BlobBinding datastructure would be the real schema for the bindings instead of having to rewrite it as this code does.

}));
map[`${name}/index.js`] = new pulumi.asset.StringAsset(`module.exports = require("./handler").handler`),
map[`${name}/handler.js`] = new pulumi.asset.StringAsset(serializedHandler.text);
// TODO: Include only the package depdencies that are referenced, similar to aws.serverless.Function
Copy link
Contributor

Choose a reason for hiding this comment

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

depdencies

@@ -151,7 +136,7 @@ export class EventSubscription<C extends Context, Data> extends pulumi.Component
args = args || {};

this.resourceGroup = args.resourceGroup || new azure.core.ResourceGroup(`${name}`, {
location: region,
location: "West US",
Copy link
Contributor

Choose a reason for hiding this comment

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

why revert this?

Copy link
Member Author

Choose a reason for hiding this comment

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

See comment in PR summary:

The hard-coding of West US is bad. I actually think we should get rid of the "automatically create a ResourceGroup" code paths here, and make users pass resource group down through all abstractions for now. It's ugly, but consistent with all other Azure APIs. If we're going to solve for that - we should likely do it in some deeper way.


const blob = new azure.storage.ZipBlob(`${name}`, {
resourceGroupName: this.resourceGroup.name,
storageAccountName: this.storageAccount.name,
storageContainerName: this.storageContainer.name,
type: "block",

content: assetMap.apply(m => new pulumi.asset.AssetArchive(m)),
// TODO: https://github.com/pulumi/pulumi-terraform/issues/191
content: new pulumi.asset.AssetArchive((assetMap as any).promise()),
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting. why does Output<AssetArchive> not work... the underlying resource code should unwrap the Output, then handle the archive... right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure - I assume just a bug somewhere - pulumi/pulumi-terraform#191 is tracking fixing the bug.

@lukehoban
Copy link
Member Author

@CyrusNajmabadi Let me know if you want to take this and address the three "lingering issues" noted in the PR summary (plus anything else needed to get this ready to merge), or if you'd like me to. Either works for me.

@CyrusNajmabadi
Copy link
Contributor

CyrusNajmabadi commented Jul 12, 2018

I'm ok with this as is. I'm going to look at trying ti figure out why outputs-of-assets+terraform aren't working properly.

@CyrusNajmabadi CyrusNajmabadi merged commit 36a805a into subscriptions Jul 25, 2018
@pulumi-bot pulumi-bot deleted the lukehoban/blobs branch July 25, 2018 19:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants