Dev ak

[Arturas-K]

Hi,

I have fixed a bug in test method that was accidentally introduced in dfc2882 in line 161, were $result is evaluated.

Also this closes #11, i have refactored parameter generation into helper method and use it in Test-TargetResource, so now Invoke-RobocopyTest is executed with the same parameters.

Also closes #12, i have updated example to use common parameter for credentials

---

This change is

[msftclas]

Hi @Arturas-K, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[Arturas-K]

This is re-implementation using parameter arrays. Now closes #6, #10, #11, #12 and #17

[narrieta]

Thank you for the update.

The code changes look good, but changing the type of AdditionalArgs would be a breaking change (wouldn't it?). Maybe keep it as string and -split it?

PS> $x = '/A /B /C' -split '\s+' PS> $x /A /B /C PS> $x.gettype().fullname System.String[] PS>

[Arturas-K]

Yes it would be breaking change. But I think it is worth it.

Splitting by space wouldn't be reliable, there are multiple options that take file name as value, if file name or any path part would have a space in it, splitter would work incorrectly.

For example if you would want to exclude several files, you would use /xf "first file" "second file" If we would implement with string splitter this configuration would not be possible, but with parameter arrays would be.

$x = '/A /B /C /XF "First file" "Second file"' -split '\s+'
$x
/A
/B
/C
/XF
"First
file"
"Second
file"

Affected options would be /XF /XD /unilog: /unilog+: /job: /save:

If we break compatibility I would suggest increasing major version in this instance which would comply with semver when you make incompatible api change.

Other option I can think of would be to add additional parameter AditionalArgsArray as an array. Mark old parameter AditionalArgs as deprecated in documentation and description. This way we would maintain compatibility and allow full usage of robocopy options via new parameter array.

Let me know what you think: do we break compatibility and increase major version or add array parameter as additional one, while deprecating old one?

[narrieta]

Yup, you are absolutely right about spaces in the arguments.

I think we will need to do the breaking change and bump the version as you suggest.

We wouldn't be able to keep the current argument, since we are getting rid of Invoke-Expression and AdditionalArgs as a string wouldn't work if there are multiple arguments.

[Arturas-K]

I would agree with this. And if anyone who is affected by this change can always get specific version from Gallery, and update their configuration in time.

[TravisEz13]

What we have been doing so far for breaking changes is marking it in bold that it is a breaking change, **Breaking Change** and the release team/person should make the version change

---

Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[Arturas-K]

Added Breaking Change to item in unreleased section.

---

Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion.

---

_DSCResources/MSFT_xRobocopy/MSFT_xRobocopy.psm1, line 177 [r1] (raw file):_ Done.

---

Comments from Reviewable