Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve display and processing of errors in Test-AzureRmResourceGroupDeployment and New-AzureRmResourceGroupDeployment #6856

Closed
ohadschn opened this issue Aug 6, 2018 · 6 comments

Comments

@ohadschn
Copy link
Contributor

ohadschn commented Aug 6, 2018

I have experienced three different ways ARM configuration issues are reported by Test-AzureRmResourceGroupDeployment

  1. Returned PSResourceManagerError as expected. The problem here is that Resolve-AzureRmError doesn't work (I guess because no ErrorRecord was created), so I had to write my own method for extracting the errors: https://stackoverflow.com/a/51713155/67824. Example:

System.Configuration.ConfigurationErrorsException: The specified KeyVault /subscriptions/1234/resourceGroups/foo/providers/Microsoft.KeyVault/vaults/bar-kv' is not in current tenant '1234' for subscription '1234'. Please see https://aka.ms/arm-keyvault for usage details. (KeyVaultParameterReferenceNotInTheSameTenant)

  1. Exception thrown. I don't think this makes sense at all, the entire point of Test-AzureRmResourceGroupDeployment is to detect ARM configuration issues and return them as PSResourceManagerError objects as documented. Example:

Resource group 'foo' could not be found.
At C:\Users\ohads\Documents\PowerShell\Test-AzureRmResourceGroupDeployment.ps1:40 char:30
... stResults = Test-AzureRmResourceGroupDeployment -ResourceGroupName $r ...
CategoryInfo CloseError: (:) [Test-AzureRmResourceGroupDeployment], CloudException
FullyQualifiedErrorId : Microsoft.Azure.Commands.ResourceManager.Cmdlets.Implementation.TestAzureResourceGroupDeploymentCmdlet

  1. User prompt for missing ARM parameters. This is an unfortunate side effect of dynamic CmdLet parameters, and in some ways it's the worst offender. When running in a non-interactive script you would only get a generic error telling you that user input isn't enabled (the error won't tell you which parameter(s) are missing). Worse, if you're waiting on some jobs, Wait-Job will fail with The Wait-Job cmdlet cannot finish working, because one or more jobs are blocked waiting for user interaction and you'll have to jump through some extra hoops to even determine which job caused the issue. An easy workaround would be a -NoDynamic flag or some such. Example:

cmdlet Test-AzureRmResourceGroupDeployment at command pipeline position 1
Supply values for the following parameters:
serviceName:

Most of the comments above are relevant for New-AzureRmResourceGroupDeployment as well.

@markcowl
Copy link
Member

markcowl commented Aug 6, 2018

@ohadschn For (1) this is by design. Resolve-AzureRmError is specifically designed for dealing with errors written by the cmdlets. I would expect Resolve-AzureRmError to handle errors from an actual (rather than test) deployment.

For (2) This is an unfortunate side-effect of how the underlying API works. Seems like the cmdlet should be a little more intellegent at handling this, (and invalid or missing template or template parameters)

For (3) I think the right option to avoid this is buy using -TemplateParameterObject or -TemplateParameterFile parameters. If you don't use these parameter sets, you are essentially tellign the cmdlet you want to use dynamic parameters to resolve template parameters.

@ohadschn
Copy link
Contributor Author

ohadschn commented Aug 6, 2018

  1. So maybe extend Resolve-AzureRmError to handle PSResourceManagerError objects? Or at least add a useful ToString implementation to the latter? I just don't think it makes sense that I have to write a function like https://stackoverflow.com/a/51713155/67824.
  2. Yeah I saw the code is shared with New-AzureRmResourceGroupDeployment, I figured this wouldn't be trivial. BTW this also complicates retry strategies - now I have to discern between exceptions that mean my configuration is wrong and exceptions that mean validation wasn't even performed (e.g. network issue).
  3. Apologies, I should have been more clear- I am using TemplateParameterFile but when parameters are missing from the file, they are prompted for.

@panchagnula panchagnula added App Services aka WebSites and removed App Services aka WebSites labels Aug 7, 2018
@maddieclayton maddieclayton added this to the 2018-08-10 milestone Aug 7, 2018
@markcowl
Copy link
Member

So for

(1) Agreed that the display of actual template deployment results from the test cmdlet should be sensible and readable from the cmdlet output.
(2) Handling simple errors like this seems like an easy enough fix.
(3) Will have to look - it should be possible to only add unsatisfied template parameters if you are in the appropriate parameter set, but this may happen too early in parameter binding for us to differentiate

@markcowl
Copy link
Member

Description

Make the following changes to Test-Deployment cmdlets

(1) Catch resourceGroup not found errors and return a valid error message in a format compatible with the rest of the depolyment errors

(2) Update default display and documentation / examples for processing resource deployment errors

(3) Determine if dynamic parameters can be omitted if a TemplateParameterObject or TeplateParamaterFiel are provided

Cost: 6

@markcowl markcowl removed their assignment Aug 14, 2018
@markcowl markcowl changed the title Inconsistent error reporting by Test-AzureRmResourceGroupDeployment Improve display and processing of errors in Test-AzureRmDeployment cmdlets Aug 14, 2018
@markcowl markcowl removed this from the 2018-08-10 milestone Aug 14, 2018
@ohadschn
Copy link
Contributor Author

Thanks! I think you switched (1) and (2) though :)

Regarding catching errors and returning them as PSResourceManagerError objects, are you sure resourceGroup not found is the only case where an exception is thrown? I just gave it as an example, more generally I was aiming for never throwing on configuration errors...

@maddieclayton maddieclayton added this to the 2018-10-19 milestone Sep 4, 2018
@bsiegel bsiegel added the Service Attention This issue is responsible by Azure service team. label Sep 26, 2018
@ohadschn ohadschn changed the title Improve display and processing of errors in Test-AzureRmDeployment cmdlets Improve display and processing of errors in Test-AzureRmResourceGroupDeployment and New-AzureRmResourceGroupDeployment Sep 27, 2018
@markcowl markcowl modified the milestones: 2018-10-23, 2018-12-04 Oct 13, 2018
@markcowl markcowl modified the milestones: 2018-12-04, 2019-01-15 Nov 19, 2018
@maddieclayton maddieclayton removed the Service Attention This issue is responsible by Azure service team. label Jan 15, 2019
@maddieclayton maddieclayton modified the milestones: 2019-01-15, 2019-01-29 Jan 15, 2019
@markcowl markcowl removed this from the 2019-01-29 milestone Jan 18, 2019
@cormacpayne
Copy link
Member

A fix for this has been merged and will be available in the next release of Az (April 9th)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants