Initial impl of cloud.Bucket for azure. #545
Conversation
azure/bucket.ts
Outdated
| const preventDestroy = opts && opts.protect; | ||
|
|
||
| const resourceGroupName = shared.resourceGroupName; | ||
| const storageAccount = shared.getGlobalStorageAccount(); |
There was a problem hiding this comment.
Current design decisions:
- there is a global resource group that can also be specified by config. This is required. We could consider making it so if you don't provide one we create one for you.
- there is a global cloud infrastructure/storage account. You can specify the name of an existing storage account you want to use (using config). if not provided we will create a new one.
There was a problem hiding this comment.
We could consider making it so if you don't provide one we create one for you.
Yes - I think we should do this. I expect the normal case here will be the user wants this to be it's own resource group.
There was a problem hiding this comment.
ok. let me try that.
There was a problem hiding this comment.
Done. Supplying a resource group is optional now. If you do, we'll use that. If you don't, we'll create a default one for you.
azure/bucket.ts
Outdated
| }; | ||
|
|
||
| this.onDelete = async (delName, handler, filter) => { | ||
| throw new Error("Method not implemented."); |
There was a problem hiding this comment.
Note: azure does not notify about blob deletes unless you use service-grid. I will file a bug on us adding support for this later.
There was a problem hiding this comment.
Should we provide a different error message here in the meantime - that this functionality isn't supported on in @pulumi/cloud-azure?
azure/examples/bucket/index.ts
Outdated
|
|
||
| const bucket = new cloud.Bucket("myBucket"); | ||
| bucket.onPut("myPutHandler", async (args) => { | ||
| console.log(JSON.stringify(args, null, 2)); |
There was a problem hiding this comment.
Note: logging to the console does not work. This is tracked over here: Azure/azure-functions-host#162
There's an open question on what we might want to do in pulumi to support this sort of thing.
There was a problem hiding this comment.
Can we just use context.log which is supported?
There was a problem hiding this comment.
Not exactly. Remember, this is in the onPut callback. onPut simply passes data about the object that was put to the callback, there is no way to get to that context object here.
Now, we have some options:
- we could pass this context object along as another arg to the callback. This would be leaky, and would mean your code was now doing very azure specific stuff. I don't like this.
- we could attempt to intercept console events and then forward them to context.log ourself. I think this would work. What do you think?
There was a problem hiding this comment.
Ok. I've gone with intercepting logging and just redirecting it to context logging.
| constructor() { | ||
| super("cloud:global:infrastructure", "global-infrastructure"); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is copied from @pulumi/aws. We could move it into @cloud/api, though i worry about this being visible in our public API.
Alternatively, we coudl maybe have a single file and symlink it to both projects.
There was a problem hiding this comment.
Its not immediately clear to me the same strategy will work for both AWS and Azure - Azure has very different name requirements. I think it's fine to allow this to be customized independently for each platform for now.
There was a problem hiding this comment.
Fair point. Will leave as a copy.
azure/shared.ts
Outdated
|
|
||
| const config = new pulumi.Config("cloud-azure"); | ||
| export const resourceGroupName = config.require("resource-group-name"); | ||
| export const location = config.require("location"); |
There was a problem hiding this comment.
design decision: the default resource-group-name and location are required config properties we need at the cloud-azure level.
There was a problem hiding this comment.
Based on talkign to luke, this has been relxed. 'location' is still required (same as aws:regoin). However, resource-group can be elided. If so, we'll create a fresh "global" resource group on the behalf of the user.
| return azure.storage.Account.get("global", storageAccountId); | ||
| } | ||
|
|
||
| return new azure.storage.Account("global", { |
There was a problem hiding this comment.
design decision: one can specify the storage account ot be used by default. if not specified, we will generate a global-infrastructure one for use in this package.
|
@lukehoban Ready for another review. Though i'll likely merge in this weekend unless you have any concerns. |
|
@lukehoban Any other concerns? |
|
Ok. Merging in. If you have any concerns let me know and i'll address them in a followup PR. |
No description provided.