[Invoke-Git] Convert to use System.Diagnostics.Process

[phbits]

Pull Request (PR) description

In a prior project, I came across AuditPolicyDsc which uses System.Diagnostics.Process to launch auditpol.exe (ref: AuditPolicyResourceHelper.psm1) as it provides more flexibility with handling error conditions. Thus converting this module is the first step toward improving error handling as mentioned in Issue #75.

I liked the readability of Publish-WikiContent and why I opted to pass the WorkingDirectory as the first argument. This leads to another benefit of not having to use Set-Location and similar commands.

This Pull Request (PR) fixes the following issues

• None

Task list

• [x] 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).
• [ ] Documentation added/updated in README.md.
• [x] Comment-based help added/updated.
• [x] Localization strings added/updated in all localization files as appropriate.
• [x] Examples appropriately added/updated.
• [x] Unit tests added/updated.
• [x] Integration tests added/updated (where possible).
• [x] New/changed code adheres to DSC Community Style Guidelines.

---

This change is 

[codecov[bot]]

Codecov Report

Merging #76 (424a333) into main (3a8ba11) will decrease coverage by 0%. The diff coverage is 100%.

@@        Coverage Diff         @@
##           main   #76   +/-   ##
==================================
- Coverage    97%   97%   -1%     
==================================
  Files        34    34           
  Lines       782   802   +20     
==================================
+ Hits        765   784   +19     
- Misses       17    18    +1     

[johlju]

I will review this a day when I have more time. 😃 But a quick view it looks like a good addition!

[phbits]

Found tests in the following are failing and will fix in the next day or two.

DscResource.DocGenerator\tests\unit\public\Publish-WikiContent.Tests.ps1

[johlju]

I will try to get to this one this weekend.

[phbits]

I will try to get to this one this weekend.

Before looking into this PR, I'd like to run something past you. There's another git call in this module:

# tasks/Publish_GitHub_Wiki_Content.build.ps1:170
$remoteURL = git remote get-url origin

If Invoke-Git is modified to return an object, it could be reused for the above instance and any future git uses.

return @{
    ExitCode       = $process.ExitCode
    StandardOutput = $process.StandardOutput.ReadToEnd()
    StandardError  = $process.StandardError.ReadToEnd()
}

Would you be interested in taking this approach? Perhaps something entirely different?

[johlju]

If we have a private function Invoke-Git that calls git then I think all calls should use the same function. 🙂

[phbits]

If we have a private function Invoke-Git that calls git then I think all calls should use the same function. 🙂

Sounds good. 🙂 This approach will be a significant change from this PR. Would you prefer to have it submitted separately?

[johlju]

Yes do it separately in another PR so it is easier to review. :)

[phbits]

Closing PR.

[johlju]

Did you close this by mistake? My thought was continue with this PR and merge it, then make a new PR with the suggestion to replace “git” with Invoke-Git in all other places. Just want to make sure I did not missunderstood 🙂

[phbits]

No, didn't close it by mistake. 🙂 I incorporated all the changes and proposed fixes from this PR into a new branch. Just opened a new PR #80 so you can take a look. Figured this would be easier/cleaner given that we both agreed on the new direction.

That being said... err, typed, since you're the maintainer I'll leave it up to you for how you want to proceed. 🙂

[johlju]

It is easier for me to continue review on this PR (branch) otherwise I need to re-review everything again. Any additional changes to other files that you proposed, that was not part of this PR, can be pushed in another PR. 😊

So please continue to fix the review comments on this PR.