Azure / PSRule.Rules.Azure

Rules to validate Azure resources and infrastructure as code (IaC) using PSRule.
https://azure.github.io/PSRule.Rules.Azure/
MIT License
394 stars 85 forks source link

Incomplete reason message for Azure.Storage.MinTLS #971

Closed BernieWhite closed 3 years ago

BernieWhite commented 3 years ago

Description of the issue

Reason message for Azure.Storage.MinTLS is not displayed correctly. Related to #970

Error output

Minimum TLS version is set to {0}.

Module in use and version:

ArmaanMcleod commented 3 years ago

@BernieWhite Wondering if a placeholder value should be inserted if the property doesn't exist? maybe Minimum TLS version is set to null.. The other option I guess is to make sure the test object has this property.

BernieWhite commented 3 years ago

The built-in reasons already do this, the issue is I set a custom reason :).

The simple solution is to remove the custom reason. The result should be one of:

ArmaanMcleod commented 3 years ago

@BernieWhite Ah makes sense. Given there are many custom reasons, should we provide a way to override customer reason with one of those messages for null properties?

I was thinking we could add some overload method like https://github.com/microsoft/PSRule/blob/9b933eeab7031a857d30aaa0ac42043d5a5044c7/src/PSRule/Runtime/AssertResult.cs#L97-L104, which would accept another parameter that would override Reason message with properties that are null.

I can see scenarios where someone would want to use a custom reason, but still be able to override them with one of the above messages if a property doesn't exist.

I havn't looked too much at the AssertResult class so this may already be possible 😄 .

BernieWhite commented 3 years ago

@ArmaanMcleod Hmm good thought. Extending of this, would something like .ReasonIf($null -eq $TargetObject.Properties.minimumTlsVersion, $LocalizedData.MinTLSVersion, 'null') work?

BernieWhite commented 3 years ago

You could already use WithReason and GetReason but you'd have to do a bit of work.

ArmaanMcleod commented 3 years ago

@BernieWhite I think that would be a great addition.