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

phbits · 2021-05-19T17:11:08Z

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

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

This change is

codecov · 2021-05-19T17:20:08Z

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

phbits

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @phbits)

source/Private/Invoke-Git.ps1, line 45 at r2 (raw file):

        if ($LASTEXITCODE -gt 1)
        {
            throw $LASTEXITCODE

I removed this throw so Publish-WikiContent can take appropriate action based on the returned ExitCode. Not entirely evident by this PR but will be used later.

phbits

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @phbits)

source/en-US/DscResource.DocGenerator.strings.psd1, line 19 at r3 (raw file):

    PushUpdatedRepoMessage              = Pushing the updated Repository to the Git Wiki.
    PublishWikiContentCompleteMessage   = Publish Wiki Content complete.
    UpdateWikiCommitMessage             = "Updating Wiki with the content for module version '{0}'."

While testing PAT permissions for the SAMPLER gist, I encountered the following error and would like to include the fix into this PR.

WARNING: Unexpected return code '1' from the following Git command.
WARNING:   PWD: C:\Users\username\AppData\Local\Temp\rccb2tj1.pap
WARNING:   git commit --message Updating Wiki with the content for module version '0.0.0.5'. --quiet
WARNING:   ERROR: error: pathspec 'Wiki' did not match any file(s) known to git
error: pathspec 'with' did not match any file(s) known to git
error: pathspec 'the' did not match any file(s) known to git
error: pathspec 'content' did not match any file(s) known to git
error: pathspec 'for' did not match any file(s) known to git
error: pathspec 'module' did not match any file(s) known to git
error: pathspec 'version' did not match any file(s) known to git
error: pathspec ''0.0.0.5'.' did not match any file(s) known to git
WARNING: There are no changes to the documentation to commit and push to the Wiki.

johlju · 2021-05-27T11:01:23Z

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

johlju

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @phbits)

source/en-US/DscResource.DocGenerator.strings.psd1, line 19 at r3 (raw file):

Previously, phbits wrote…

While testing PAT permissions for the SAMPLER gist, I encountered the following error and would like to include the fix into this PR.

WARNING: Unexpected return code '1' from the following Git command.
WARNING:   PWD: C:\Users\username\AppData\Local\Temp\rccb2tj1.pap
WARNING:   git commit --message Updating Wiki with the content for module version '0.0.0.5'. --quiet
WARNING:   ERROR: error: pathspec 'Wiki' did not match any file(s) known to git
error: pathspec 'with' did not match any file(s) known to git
error: pathspec 'the' did not match any file(s) known to git
error: pathspec 'content' did not match any file(s) known to git
error: pathspec 'for' did not match any file(s) known to git
error: pathspec 'module' did not match any file(s) known to git
error: pathspec 'version' did not match any file(s) known to git
error: pathspec ''0.0.0.5'.' did not match any file(s) known to git
WARNING: There are no changes to the documentation to commit and push to the Wiki.

Can we add those double-quotes in the code instead? Otherwise I'm afraid they are removed by someone else thinking it is a typo. 🙂

phbits · 2021-06-01T22:45:26Z

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 · 2021-06-03T09:25:01Z

I will try to get to this one this weekend.

phbits · 2021-06-03T16:11:41Z

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 · 2021-06-08T04:35:05Z

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

johlju

Other than improving error handling - will this change improve anything else for the tasks? 🤔 Is there a problem that this change will help improve (eventually)?

Reviewed 2 of 6 files at r1, 1 of 2 files at r4.
Reviewable status: 3 of 6 files reviewed, 13 unresolved discussions (waiting on @phbits)

source/Private/Invoke-Git.ps1, line 45 at r2 (raw file):

Previously, phbits wrote…

I removed this throw so Publish-WikiContent can take appropriate action based on the returned ExitCode. Not entirely evident by this PR but will be used later.

I think by default this function should throw on error to stop the pipeline if git returns an error. If there should be more processing instead of throwing we can add a parameter PassThru that will return an object for more processing. Then a specific call can opt-in to not throw.

source/Private/Invoke-Git.ps1, line 24 at r5 (raw file):

$workingDirectory = $Arguments[0]

We should add a parameter WorkingDirectory. Since the function is not exported public it is not necessary to use positional parameters since we should use named parameters to follow style guideline. I see now that this function has not followed the style guideline. :)

So the call to Invoke-Git should look like this throughout:

Invoke-Git -WorkingDirectory 'MyFolder' -Arguments @('config', '--global', 'core.autocrlf', 'true')

source/Private/Invoke-Git.ps1, line 28 at r5 (raw file):

Quoted 4 lines of code…

    for ($i=1; $i -lt $Arguments.Length; $i++)
    {
        [string[]] $cmdArguments += $Arguments[$i]
    }

This should not be needed if we add parameter in previous comment.

source/Private/Invoke-Git.ps1, line 43 at r5 (raw file):

New-Object System.Diagnostics.Process

We should use named parameters, e.g. New-Object -TypeName 'System.Diagnostics.Process'.

source/Private/Invoke-Git.ps1, line 56 at r5 (raw file):

 -1 specifies an infinite wait.

I see a potential that this will keep a build worker running unnecessary long if there is an error. We should add a parameter Timeout with a default value for this instead. Default value can be 120 seconds, it should be sufficient to do most Git work.

If that is not enough time, then another PR can add the possibility to change the value through the build configuration.

source/Private/Invoke-Git.ps1, line 70 at r5 (raw file):

                    Write-Warning -Message "  PWD: $workingDirectory"
                    Write-Warning -Message "  git $argumentsJoined"

Can we add these to the localized string warning message above instead of having separate entries. Though, these looks like more as Write-Debug messages?

source/Private/Invoke-Git.ps1, line 72 at r5 (raw file):

[string]

We should use full name [System.String]. Same on the line below too.

source/Private/Invoke-Git.ps1, line 77 at r5 (raw file):

 Write-Warning -Message "  OUTPUT: $invokeGitOutput"

What are your thoughts here? Why are we writing the standard output from git as a warning?

source/Private/Invoke-Git.ps1, line 79 at r5 (raw file):

if ([System.String]::IsNullOrWhiteSpace($invokeGitError) -eq $false)

We should add a new line before this line.

source/Private/Invoke-Git.ps1, line 87 at r5 (raw file):

catch {

We should have open brace on a separate line.

source/Private/Invoke-Git.ps1, line 90 at r5 (raw file):

        $e = $_
        Write-Error -Message $e.Exception.Message

It should be enough to do throw $_

source/Private/Invoke-Git.ps1, line 92 at r5 (raw file):

finally {

We should have open brace on a separate line.

source/Private/Invoke-Git.ps1, line 95 at r5 (raw file):

        $exitCode = $process.ExitCode
        $process.Dispose()

Maybe we should check that $process is not null here if for some reason the New-Objectfailed?

phbits · 2021-06-08T17:09:42Z

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 · 2021-06-08T17:46:29Z

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

phbits

Ok. Thanks for the feedback here. I've incorporated it into the new branch.

Reviewable status: 3 of 6 files reviewed, 13 unresolved discussions (waiting on @johlju and @phbits)

source/Private/Invoke-Git.ps1, line 45 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

I think by default this function should throw on error to stop the pipeline if git returns an error. If there should be more processing instead of throwing we can add a parameter PassThru that will return an object for more processing. Then a specific call can opt-in to not throw.

Agreed. Now that an object is being returned, if there's an error it will throw.

source/Private/Invoke-Git.ps1, line 24 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$workingDirectory = $Arguments[0]
We should add a parameter WorkingDirectory. Since the function is not exported public it is not necessary to use positional parameters since we should use named parameters to follow style guideline. I see now that this function has not followed the style guideline. :)

So the call to Invoke-Git should look like this throughout:
Invoke-Git -WorkingDirectory 'MyFolder' -Arguments @('config', '--global', 'core.autocrlf', 'true')

Done.

source/Private/Invoke-Git.ps1, line 43 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

New-Object System.Diagnostics.Process
We should use named parameters, e.g. New-Object -TypeName 'System.Diagnostics.Process'.

Done.

source/Private/Invoke-Git.ps1, line 56 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

 -1 specifies an infinite wait.
I see a potential that this will keep a build worker running unnecessary long if there is an error. We should add a parameter Timeout with a default value for this instead. Default value can be 120 seconds, it should be sufficient to do most Git work.

If that is not enough time, then another PR can add the possibility to change the value through the build configuration.

Done.

source/Private/Invoke-Git.ps1, line 70 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

                    Write-Warning -Message "  PWD: $workingDirectory"
                    Write-Warning -Message "  git $argumentsJoined"
Can we add these to the localized string warning message above instead of having separate entries. Though, these looks like more as Write-Debug messages?

Agreed. I was trying to keep with the theme of handling ExitCode > 1 situations softer than going straight to throw. I've removed this entirely so the calling entry can take appropriate action.

source/Private/Invoke-Git.ps1, line 72 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

[string]
We should use full name [System.String]. Same on the line below too.

Done.

source/Private/Invoke-Git.ps1, line 77 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

 Write-Warning -Message "  OUTPUT: $invokeGitOutput"
What are your thoughts here? Why are we writing the standard output from git as a warning?

As mentioned above, it was my way of handling ExitCode > 1.

source/Private/Invoke-Git.ps1, line 79 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if ([System.String]::IsNullOrWhiteSpace($invokeGitError) -eq $false)
We should add a new line before this line.

Done.

source/Private/Invoke-Git.ps1, line 87 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

catch {
We should have open brace on a separate line.

Done.

source/Private/Invoke-Git.ps1, line 90 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

        $e = $_
        Write-Error -Message $e.Exception.Message

It should be enough to do throw $_

Done.

source/Private/Invoke-Git.ps1, line 92 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

finally {
We should have open brace on a separate line.

Done.

source/Private/Invoke-Git.ps1, line 95 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

        $exitCode = $process.ExitCode
        $process.Dispose()
Maybe we should check that $process is not null here if for some reason the New-Objectfailed?

Okay. Just to be safe I'll update it to:

if ($process)
{
    $process.Dispose()
}

phbits · 2021-06-08T20:52:17Z

Closing PR.

johlju · 2021-06-09T05:12:55Z

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 · 2021-06-10T17:23:58Z

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 · 2021-06-10T18:00:18Z

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.

johlju

Reviewed 5 of 5 files at r6, 1 of 1 files at r7.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @phbits)

source/Private/Invoke-Git.ps1, line 45 at r2 (raw file):

Previously, phbits wrote…

Agreed. Now that an object is being returned, if there's an error it will throw.

If this function is being modified for the caller to handle any error based of the exit code, then should we output a warning? Shouldn't it also be up to the caller to output any warning or throw an error? 🤔

source/Private/Invoke-Git.ps1, line 14 at r7 (raw file):

Quoted 4 lines of code…

    .EXAMPLE
        Invoke-Git clone https://github.com/X-Guardian/xActiveDirectory.wiki.git --quiet

        Invokes the Git executable to clone the specified repository to the current working directory.

Can we update this example with the named parameters (and the mandatory one).

tests/unit/private/Invoke-Git.Tests.ps1, line 37 at r7 (raw file):

$WorkingDirectory.FullName

Can't we just use $TestDrive here directly? Throughout.

phbits

Reviewable status: 2 of 6 files reviewed, 3 unresolved discussions (waiting on @johlju)

source/Private/Invoke-Git.ps1, line 45 at r2 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

If this function is being modified for the caller to handle any error based of the exit code, then should we output a warning? Shouldn't it also be up to the caller to output any warning or throw an error? 🤔

I agree. Unfortunately, this PR doesn't have Invoke-Git return an object, only the return code. The additional information, StandardOutput & StandardError, is needed for the caller and fixed in PR #80. This PR maintains pieces currently in use which is why it still returns the ExitCode and utilizes Write-Warning and Write-Debug. I cleaned this portion up in the latest commit which for some reason isn't showing up in Reviewable. 🤔 Once the latest revision shows up I think you'll find it suitable for this PR.

source/Private/Invoke-Git.ps1, line 14 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

    .EXAMPLE
        Invoke-Git clone https://github.com/X-Guardian/xActiveDirectory.wiki.git --quiet

        Invokes the Git executable to clone the specified repository to the current working directory.

Can we update this example with the named parameters (and the mandatory one).

Good catch!

tests/unit/private/Invoke-Git.Tests.ps1, line 37 at r7 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$WorkingDirectory.FullName
Can't we just use $TestDrive here directly? Throughout.

Done.

johlju

Reviewed 4 of 4 files at r8.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @phbits)

source/Private/Invoke-Git.ps1, line 45 at r2 (raw file):

Previously, phbits wrote…

I agree. Unfortunately, this PR doesn't have Invoke-Git return an object, only the return code. The additional information, StandardOutput & StandardError, is needed for the caller and fixed in PR #80. This PR maintains pieces currently in use which is why it still returns the ExitCode and utilizes Write-Warning and Write-Debug. I cleaned this portion up in the latest commit which for some reason isn't showing up in Reviewable. 🤔 Once the latest revision shows up I think you'll find it suitable for this PR.

Lets' fix this in the next PR then.

phbits added 3 commits May 17, 2021 15:59

Converted Invoke-Git cmdlet to use System.Diagnostics.Process

590f74f

Updated Unit tests and Invoke-Git

721df1a

fix unit tests

084ca18

remove typeo

1f57dd2

phbits commented May 20, 2021

View reviewed changes

add quotes to localized commit msg

875c5a6

phbits commented May 21, 2021

View reviewed changes

johlju added the needs review The pull request needs a code review. label May 27, 2021

johlju reviewed May 27, 2021

View reviewed changes

use double-quotes with git commit

ecdc63a

fix unit test

517da02

johlju requested changes Jun 8, 2021

View reviewed changes

phbits commented Jun 8, 2021

View reviewed changes

phbits closed this Jun 8, 2021

johlju reopened this Jun 9, 2021

johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Jun 10, 2021

reviewable edits

5722d79

Merge branch 'main' into dev

50ae467

johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Jun 13, 2021

johlju requested changes Jun 13, 2021

View reviewed changes

johlju added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels Jun 16, 2021

phbits added 2 commits June 17, 2021 18:06

reviewable edits

506b2fe

Merge branch 'main' into dev

424a333

phbits commented Jun 17, 2021

View reviewed changes

johlju approved these changes Jun 19, 2021

View reviewed changes

johlju merged commit c03c040 into dsccommunity:main Jun 19, 2021

johlju removed the waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. label Jun 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

phbits commented May 19, 2021 •

edited by johlju

Loading

codecov bot commented May 19, 2021 •

edited

Loading

phbits left a comment

phbits left a comment

johlju commented May 27, 2021

johlju left a comment

phbits commented Jun 1, 2021

johlju commented Jun 3, 2021

phbits commented Jun 3, 2021

johlju commented Jun 8, 2021

johlju left a comment

phbits commented Jun 8, 2021

johlju commented Jun 8, 2021

phbits left a comment

phbits commented Jun 8, 2021

johlju commented Jun 9, 2021

phbits commented Jun 10, 2021

johlju commented Jun 10, 2021

johlju left a comment

phbits left a comment

johlju left a comment

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

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

Conversation

phbits commented May 19, 2021 • edited by johlju Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

codecov bot commented May 19, 2021 • edited Loading

Codecov Report

phbits left a comment

Choose a reason for hiding this comment

phbits left a comment

Choose a reason for hiding this comment

johlju commented May 27, 2021

johlju left a comment

Choose a reason for hiding this comment

phbits commented Jun 1, 2021

johlju commented Jun 3, 2021

phbits commented Jun 3, 2021

johlju commented Jun 8, 2021

johlju left a comment

Choose a reason for hiding this comment

phbits commented Jun 8, 2021

johlju commented Jun 8, 2021

phbits left a comment

Choose a reason for hiding this comment

phbits commented Jun 8, 2021

johlju commented Jun 9, 2021

phbits commented Jun 10, 2021

johlju commented Jun 10, 2021

johlju left a comment

Choose a reason for hiding this comment

phbits left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

phbits commented May 19, 2021 •

edited by johlju

Loading

codecov bot commented May 19, 2021 •

edited

Loading