dsccommunity / SqlServerDsc

This module contains DSC resources for deployment and configuration of Microsoft SQL Server.
MIT License
355 stars 225 forks source link

SqlServerDatabaseMail: Resource should not impose defaults for DisplayName and ReplyToAddress #1216

Open NReilingh opened 5 years ago

NReilingh commented 5 years ago

CC @johlju re: https://github.com/powershell/sqlserverdsc/pull/1200#pullrequestreview-151268987

Details of the scenario you tried and the problem that is occurring

The resource currently defines default values for the parameters DisplayName and ReplyToAddress. This causes these values to be explicitly set on the SMO Mail.MailAccount object, which causes DatabaseMail to apply a different treatment to outgoing email headers than if these values were unset.

The email headers that result will usually cause a receiving email client to behave the same way as if the values were unset, but this is an implementation detail that really isn't up to us, and we should instead be focused on the actual email headers being consistent whether this feature is configured manually or programmatically with this DSC resource.

Verbose logs showing the problem

In the event that these properties are unset or not provided during a manual (wizard-based) configuration of DatabaseMail, outgoing email headers will contain the following:

From: sender@example.com
To: recipient@example.com

However, if the DSC resource explicitly sets default values and configures them, outgoing email headers will contain:

From: "sender@example.com" <sender@example.com>
To: recipient@example.com
Reply-To: sender@example.com

Suggested solution to the issue

I believe all that would need to be done is to remove the = $EmailAddress parameter default declarations from Set-TargetResource and Test-TargetResource — when testing by manipulating the SMO objects in PowerShell, setting the object property to an undefined variable had the desired effect of "unsetting" the property. I assume that Test-SQLDscParameterState is smart enough to compare blank/unset/empty string/null properly — currently I am not smart enough to know the subtleties of how PowerShell treats them. ;-)

The documentation would also be updated to indicate that this DSC resource does not impose its own default behavior for these properties and instead leaves it to SQL server.

The DSC configuration that is used to reproduce the issue (as detailed as possible)

N/A

SQL Server edition and version the target node is running

N/A

SQL Server PowerShell modules present on the target node

N/A

The operating system the target node is running

N/A

Version and build of PowerShell the target node is running

N/A

Version of the DSC module that was used ('dev' if using current dev branch)

dev

johlju commented 5 years ago

Thanks for the detailed information in this issue! 🙂 Labeled this as enhancement and help wanted.

Should we consider this a breaking change? My first thought is not, because if the the default value is removed, that property is not part of the configuration, and since it is not part of the configuration, the resource should not try to remove for example DisplayName and set it to $null (nothing). It is a breaking change in sense that it will behave differently next time the same configuration is run. Leaning towards not making this a breaking change? Any other opinions, either way?

I assume that Test-SQLDscParameterState is smart enough to compare blank/unset/empty string/null properly

We can add regression tests (unit tests) that verifies that we get the correct values from Test-TargetResource (and Get-TargetResource).