ADReplicationSiteLink: Fix Setting Options After the Resource is Initially Created

[X-Guardian]

Pull Request (PR) description

This PR fixes the issue with the ADReplicationSiteLink resource whereby setting OptionChangeNotification, OptionTwoWaySync or OptionDisableCompression fails after the resource is initially created.

This Pull Request (PR) fixes the following issues

• Fixes ADReplicationSiteLink: Fails to Set Options After the Resource is Initially Created  #605

Task list

• Added an entry to the change log under the Unreleased section of the
  file CHANGELOG.md. Entry should say what was changed and how that
  affects users (if applicable), and reference the issue being resolved
  (if applicable).
• Resource documentation added/updated in README.md.
• Resource parameter descriptions added/updated in README.md, schema.mof
  and comment-based help.
• Comment-based help added/updated.
• Localization strings added/updated in all localization files as appropriate.
• Examples appropriately added/updated.
• Unit tests added/updated. See DSC Community Testing Guidelines.
• Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
• New/changed code adheres to DSC Community Style Guidelines.

---

This change is 

[johlju]

Reviewed 1 of 3 files at r1.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @johlju and @X-Guardian)

---

source/DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 292 at r1 (raw file):

if ($PSBoundParameters.ContainsKey('OptionChangeNotification') 

This was used to only change this property if it was passed in the configuration (if the value should be enforced on an existing site link). With this change it looks like the resource will always be changed regardless if the configuration wanted to enforce that value or not.

If the values need to be set when initially creating the site link, then that should be separate logic from enforcing an existing site link?

[X-Guardian]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @johlju)

---

source/DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 292 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if ($PSBoundParameters.ContainsKey('OptionChangeNotification') 

This was used to only change this property if it was passed in the configuration (if the value should be enforced on an existing site link). With this change it looks like the resource will always be changed regardless if the configuration wanted to enforce that value or not.

If the values need to be set when initially creating the site link, then that should be separate logic from enforcing an existing site link?

The parameters have a default value, which we should be applying if not specified in the config, which is what this change does. If we don't want them to have a default value, we should remove the defaults from the parameter definitions.

[johlju]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @johlju and @X-Guardian)

---

source/DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 292 at r1 (raw file):

Previously, X-Guardian (Simon Heather) wrote…

The parameters have a default value, which we should be applying if not specified in the config, which is what this change does. If we don't want them to have a default value, we should remove the defaults from the parameter definitions.

I think we should remove the default values, because I think we should only enforce values that is part of a configuration. The proposed change would be a breaking change since it will start enforcing values not in an existing configuration. If we remove the default values then this would not be a breaking change. 🤔

What do you think?

[X-Guardian]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @johlju)

---

source/DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 292 at r1 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I think we should remove the default values, because I think we should only enforce values that is part of a configuration. The proposed change would be a breaking change since it will start enforcing values not in an existing configuration. If we remove the default values then this would not be a breaking change. 🤔

What do you think?

Agreed, Johan. We shouldn't have non-required properties with default values, as it is confusing as to whether they are 'managed' properties or not. I'll remove the default values and refactor the code.

[X-Guardian]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @johlju)

---

source/DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 292 at r1 (raw file):

Previously, X-Guardian (Simon Heather) wrote…

Agreed, Johan. We shouldn't have non-required properties with default values, as it is confusing as to whether they are 'managed' properties or not. I'll remove the default values and refactor the code.

Done.

[johlju]

Reviewed 1 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved

[codecov]

Codecov Report

Merging #606 into master will increase coverage by 0%.
The diff coverage is 98%.

@@          Coverage Diff          @@
##           master   #606   +/-   ##
=====================================
  Coverage      98%    99%           
=====================================
  Files          23     23           
  Lines        3093   3045   -48     
=====================================
- Hits         3059   3031   -28     
+ Misses         34     14   -20