jeffhollan / LogicAppTemplateCreator

Script to convert Logic Apps into templates for deployment
MIT License
143 stars 74 forks source link

DiagnosticSettings as switch parameter #55

Closed Splaxi closed 4 years ago

Splaxi commented 5 years ago

From a powershell perspective, it makes most sense that when we have bool parameters that are default false, they should be of the type SwitchParameter.

This enables the user to just add the parameter as a switch, instead of having to add the parameter and parse $true to them.

E.g. -DiagnosticSettings:$true becomes -DiagnosticSettings

This implementation should be more consistent with the powershell paradigm and it is actually mentioned in the docs that we should try to use the SwitchParameter instead of bool / boolean.

https://docs.microsoft.com/en-us/powershell/developer/cmdlet/types-of-cmdlet-parameters#switch-parameters

Splaxi commented 4 years ago

Could you share an example that you think would break?

Makes it easier for me to find a good solution 😊

MLogdberg commented 4 years ago

Just tested my last used script: got this error: Get-LogicAppTemplate : A positional parameter cannot be found that accepts argument 'True'. At C:\temp\GenerateArmTemplates\GetLogicApp - Privat.ps1:23 char:1

Maybee it's just me overanalyzing but this will force the one using the script to change it. (not a big thing, just wanted to point out, I just might need to point that out :) )

Splaxi commented 4 years ago

Could you share your command, including the parameters? The values can be random / masked.

I support you use position parameters, but I just want to make sure how you Invoke the command...

MLogdberg commented 4 years ago

So my is looking like this (using the newly added just for showing) so this will fail (error above) Get-LogicAppTemplate -LogicApp $name -ResourceGroup $resourcegroupname -SubscriptionId $subscriptionid -TenantName $tenant -IncludeInitializeVariable $true | Out-File $filenname

Fixing it is easy: Get-LogicAppTemplate -LogicApp $name -ResourceGroup $resourcegroupname -SubscriptionId $subscriptionid -TenantName $tenant -IncludeInitializeVariable | Out-File $filenname

Might just need to write this in a blog poast or so...

Splaxi commented 4 years ago

Ah, I see!

That is because the switch is a bit different than the boolean.

The last command is the most correct / native powershell way to do it.

But you could also do:

... -IncludeInitializeVariable:$true ...

The reason is that you got use to pass a parameter value to the parameter, because it was a boolean. Now it is a switch, which doesn't accept a value in the same way.

Remember that we can write it the Get-Help examples and explain it, so that people who did pass a value earlier learns how to do it properly going forward 😀

MLogdberg commented 4 years ago

Sounds good to extend the help with this!