Enable new workspace re-deploy settings for deployWorkspaceProject#454
Enable new workspace re-deploy settings for deployWorkspaceProject#454MicroFish91 merged 11 commits intomainfrom
deployWorkspaceProject#454Conversation
MicroFish91
changed the title
deployWorkspaceProjectdeployWorkspaceProject
…de-azurecontainerapps into mwf/deployWorkspaceProject-V-I
…inerapps into mwf/deployWorkspaceProject-V-I
| containerRegistryName | ||
| }; | ||
| } | ||
| } catch { /** Do nothing */ } |
There was a problem hiding this comment.
Might be useful to add a comment here that explains what errors are being ignored here and why
There was a problem hiding this comment.
Ok now that I see this again, you should probably log something inside that catch block too.
There was a problem hiding this comment.
This was a good suggestion, especially because it actually pushed me to test the get setting method a little bit more 😆
I realized that it doesn't actually throw an error even with bad settings path and property... so I removed the catch entirely. I also made a small change to the setting utils so that it reads a missing value as undefined rather than as an empty string '' (definitely not a behavior I expected it to have by default).
| const savedRegistry: Registry | undefined = registries.find(r => r.name === settings.containerRegistryName); | ||
|
|
||
| if (savedRegistry) { | ||
| ext.outputChannel.appendLog(localize('foundResourceMatch', 'Used saved workspace settings and found an existing container registry.')); |
There was a problem hiding this comment.
Big fan of the logs in this function. You could include the found resource names in the log if you wanted to be even more verbose. But just an idea.
There was a problem hiding this comment.
This suggestion will be included in the following PR!
| const containerAppResourceGroupName: string | undefined = settingUtils.getWorkspaceSetting(`${deployWorkspaceProjectPrefix}.containerAppResourceGroupName`, settingsPath); | ||
| const containerRegistryName: string | undefined = settingUtils.getWorkspaceSetting(`${deployWorkspaceProjectPrefix}.containerRegistryName`, settingsPath); | ||
|
|
||
| if (containerAppName || containerAppResourceGroupName || containerRegistryName) { |
There was a problem hiding this comment.
This isn't a blocker, but I think it's awkward to only return an object if one of the properties is defined. And otherwise we return undefined.
In the future, if you add more settings, then you need to add them to this if expression.
I think I'd prefer if you just always return an object, even if the object just contains undefined values.
MicroFish91
force-pushed
the
mwf/deployWorkspaceProject-V-I
branch
2 times, most recently
from
19ab5a8 to
206ab5b
Compare
MicroFish91
force-pushed
the
mwf/deployWorkspaceProject-V-I
branch
from
206ab5b to
ad4f149
Compare
MicroFish91
force-pushed
the
mwf/deployWorkspaceProject-V-I
branch
from
d9e378f to
052fc86
Compare
2 participants
Related to #425
Another partial PR for
deployWorkspaceProject.Added support for 3 new (re-)deploy settings for
deployWorkspaceProject. We use these three settings to check for existing resources to add to the context before executing the deployment.The default resources affected by this PR are: container app, managed environment, resource group, ACR.
Logic goes:
undefinedand allow theDefaultResourcesNameStep(added in previous PR) to name the resources for later creationThe workflow in general has been demo'd to Anthony and Misty including the showing of a re-deploy scenario with these same settings. Feel free to suggest any improvements as necessary though.
What the full thing will eventually look like: demo