Improve local settings when creating templates #3924
Conversation
| @@ -33,7 +33,10 @@ export abstract class AzureConnectionCreateStepBase<T extends IBindingWizardCont | |||
| progress.report({ message: localize('retrieving', 'Retrieving connection string...') }); | |||
|
|
|||
| const result: IConnection = await this.getConnection(context); | |||
| let appSettingKey: string = `${result.name}_${nonNullProp(this._setting, 'resourceType').toUpperCase()}`; | |||
| let appSettingKey: string = context.useStorageEmulator ? | |||
| // if using the storage emulator, don't append the resource type to the app setting key | |||
There was a problem hiding this comment.
nit: The comment explains what we're doing but not why. I think it's more helpful if the why was included.
There was a problem hiding this comment.
To be honest, I'm not really sure why @ejizba did this in the first place.
https://learn.microsoft.com/en-us/azure/azure-functions/functions-app-settings#azurewebjobsstorage
Here's the documentation about the setting AzureWebJobsStorage which is the default app setting linking to a storage account.
The old behavior would take the name of the azure resource and append the resource type (so like, naturins-storage_STORAGE) as the key. I'm wondering if AzureWebJobsStorage didn't exist at that time so he was just finding a common convention for the key?
There was a problem hiding this comment.
Not sure if he's talking about what I did or what you did @nturinski. The "_STORAGE" thing was just a random naming convention and I feel like I was probably just following an existing pattern from docs or the portal
In any case, I'm confused about what you're doing just like Alex probably is. Like, what does the emulator have to do with appending "_STORAGE"? Seems like both "AzureWebJobsStorage" and "randomkey_STORAGE" could use or not use the emulator, right?
There was a problem hiding this comment.
The issue is when I pass in AzureWebJobsStorage as the app key, it'll automatically append _STORAGE to the end of it. You can see it in my screenshot where that happened.
Is it pretty common to have other storage connection strings outside of AzureWebJobsStorage?
package.json
Outdated
| @@ -276,11 +276,6 @@ | |||
| "title": "%azureFunctions.restartFunctionApp%", | |||
| "category": "Azure Functions" | |||
| }, | |||
| { | |||
| "command": "azureFunctions.setAzureWebJobsStorage", | |||
There was a problem hiding this comment.
Also setAzureWebJobsStorage is never actually invoked anywhere, so I went ahead and deleted that.
I use it a decent amount please don't remove it 🥺 If you clone a function app repo or just accidentally delete your "local.settings.json" it's a fast way to get the file back
There was a problem hiding this comment.
Hahaha okay. I figured the only entry point was through the command palette, so who could even be using it...? But I guess you are 1 of 85 people per month.
| let picks: IAzureQuickPickItem<string | undefined>[] = [{ label: localize('newAppSetting', '$(plus) Create new local app setting'), data: undefined }]; | ||
| picks = picks.concat(existingSettings.map((s: string) => { return { data: s, label: s }; })); | ||
| const placeHolder: string = localize('selectAppSetting', 'Select setting from "{0}"', localSettingsFileName); | ||
| picks = picks.concat(existingSettings.map((s: [string, string]) => { return { data: s[0], label: s[0], description: s[1] }; })); |
There was a problem hiding this comment.
I added the value to the list of local settings.
The value can be a secret, though. You could add an option or something to show values, but they should be hidden by default just to be safe. Just like how app settings are currently displayed in the tree view
|
Not sure who's approval I'm seeking now... |
src/commands/registerCommands.ts
Outdated
| @@ -93,7 +92,6 @@ export function registerCommands(): void { | |||
| registerCommandWithTreeNodeUnwrapping('azureFunctions.pickProcess', pickFuncProcess); | |||
| registerCommandWithTreeNodeUnwrapping('azureFunctions.redeploy', redeployDeployment); | |||
| registerCommandWithTreeNodeUnwrapping('azureFunctions.restartFunctionApp', restartFunctionApp); | |||
| registerCommandWithTreeNodeUnwrapping('azureFunctions.setAzureWebJobsStorage', setAzureWebJobsStorage); | |||
There was a problem hiding this comment.
Don't we still need this for the command palette to work?
There was a problem hiding this comment.
You are absolutely correct. Forgot to revert that one. 🤦
| export async function getStorageConnectionString(context: IStorageAccountWizardContext & { useStorageEmulator?: boolean }): Promise<IResourceResult> { | ||
| if (context.useStorageEmulator) { | ||
| return { | ||
| name: 'AZURITE', |
There was a problem hiding this comment.
I feel like "AZURITE" as a name isn't ideal because the same name is used when you deploy to Azure and you obviously can't use Azurite in Azure. I'd pick a more generic name that makes sense for both local and remote scenarios
Some random ideas:
- connection1_STORAGE
- {randomHex}_STORAGE (for example a8b382_STORAGE)
- {functionName}Connection (for example blobTrigger1Connection)
- storageConnection1
There was a problem hiding this comment.
To be clear: my suggestions were for the end result of the app setting name, not necessarily this particular "name" variable as I'm not exactly sure how it's used.
There was a problem hiding this comment.
Going with randomHex because it's easiest and don't need to keep track of duplicates or anything like that,
src/commands/appSettings/connectionSettings/getLocalConnectionSetting.ts
Show resolved
Hide resolved
I added the value to the list of local settings. I think it's helpful to see what those are to give more context. Also letting users select emulator vs. remote when adding a new local setting for a storage connection string.
I still need to do this for the v2 schema since they don't use bindings anymore. It should be pretty similar, but I'll do it in a different PR.
Also
setAzureWebJobsStorageis never actually invoked anywhere, so I went ahead and deleted that.