Complete support for Releases API #177

HowardWolosky · 2020-05-22T23:59:09Z

Description

This completes the required work to support the set of Releases API's.

It adds the following functions:

New-GitHubRelease
Set-GitHubRelease
Remove-GitHubRelease
Get-GitHubReleaseAsset
New-GitHubReleaseAsset
Set-GitHubReleaseAsset
Remove-GitHubReleaseAsset

Invoke-GHRestMethod has been updated to be able to upload a file (via the new InFile parameter) and download a file (via the Save switch which will cause it to return back a FileInfo object of a temporary file which can then be renamed as seen fit by the caller).

This also adds formatters for GitHub.Release and GitHub.ReleaseAsset.

Positional Binding has been set as false for the three functions, and Position attributes added to the function's mandatory parameters.

Issues Fixed

Fixes #47
Fixes #110

References

GitHub Releases

Checklist

You actually ran the code that you just wrote, especially if you did just "one last quick change".
Comment-based help added/updated, including examples.
Static analysis is reporting back clean.
New/changed code adheres to our coding guidelines.
Changes to the manifest file follow the manifest guidance.
Unit tests were added/updated and are all passing. See testing guidelines.
Relevant usage examples have been added/updated in USAGE.md.
~~If desired, ensure your name is added to our Contributors list~~

rjmholt · 2020-06-07T00:41:40Z

Ah, glad I saw this. Had begun adapting this to contribute here

rjmholt · 2020-06-07T00:44:42Z

GitHubCore.ps1

@@ -130,6 +148,32 @@ function Invoke-GHRestMethod

    Invoke-UpdateCheck

+    # Minor error checking around $InFile
+    if ((-not [String]::IsNullOrWhiteSpace($InFile)) -and ($Method -ne 'Post'))


Is there a reason for using [string]::IsNullOrWhitespace() rather than $PSBoundParameter.ContainsKey()?

My thinking is that a [ValidateNotNullOrEmpty()] attribute will guarantee that some parameter value is passed, and checking $PSBoundParameters is a better way to distinguish whether the parameter was set (rather than an empty value being passed in)

That's fair. I think my memory had flaked here on whether that could be used successfully on an optional parameter. I just verified that it would work fine. Will change.

function Invoke-NotNull { [cmdletbinding()] param( [ValidateNotNullOrEmpty()] [string] $Value ) write-host "[$Value]" } Invoke-NotNull [] Invoke-NotNull -Value 'foo' [foo] Invoke-NotNull -Value $null Invoke-NotNull : Cannot validate argument on parameter 'Value'.

The scenario I have specifically in my head is something like:

New-GitHubRelease ... -Label $myLabel

in a case where I forgot to set $myLabel

in a case where I forgot to set $myLabel

There is no Label for that function, so I'll assume your example was supposed to be -Description $myDescription.

Ah, so this is where it gets tricky. It's valid to have no description. I consider it a feature that earlier logic in your script might have determined that the description was $null and now you're just blindly passing it in. We'll take that input and just ignore it because [String]::IsNullOrEmpty. Is there a more consistent PowerShell philosophy in existing cmdlets where it doesn't work that way?

Ah, so this is where it gets tricky. It's valid to have no description. I consider it a feature that earlier logic in your script might have determined that the description was $null and now you're just blindly passing it in.

Yeah, that's exactly why I'd use [ValidateNotNullOrEmpty()] on a non-mandatory parameter, to get this behaviour:

> function Test { param([ValidateNotNullOrEmpty()][Parameter()]$x) $x } > Test > Test -x $notSet Test: Cannot validate argument on parameter 'x'. The argument is null or empty. Provide an argument that is not null or empty, and then try the command again. > Test -x banana banana

This way, it's perfectly valid to not specify the parameter, but if you do specify it, it shouldn't be the same as not specifying it.

The only issue there is that that means that more automated uses where the parameter is constructed, for example with splatting, have to be slightly more careful, since having a default value of null or empty string will cause a parameter binding failure.

Anyway, point is that it's totally philosophical, but just wanted to make sure it's a deliberate choice

The only issue there is that that means that more automated uses where the parameter is constructed, for example with splatting, have to be slightly more careful, since having a default value of null or empty string will cause a parameter binding failure.

That's the point I was trying to drive at. It seems like it's a positive feature to allow $null to be sent in and thus have the parameter ignored. But I'm just a single user, and so I am interested in hearing from others if they feel similarly or not.

Yeah, I think that's a totally valid point.

My personal usage, especially of a module like this, is a lot more interactive, and tends to involve something like executing lines that I've already written. But if I get them out of order and it quietly goes ahead, I have to go and fix it on GitHub (perhaps the lesson is that I should use -WhatIf...).

I don't feel that strongly about it. My real 2 cents are that it's less of a breaking change to turn it from an error into something that works later than to go the other way.

GitHubCore.ps1

GitHubReleases.ps1

rjmholt · 2020-06-07T01:36:51Z

GitHubReleases.ps1

+    $Path = Resolve-UnverifiedPath -Path $Path
+    $file = Get-Item -Path $Path
+    $fileName = $file.Name
+    $fileNameEncoded = [System.Web.HTTPUtility]::UrlEncode($fileName)


Consider [uri]::EscapeDataString() instead

rjmholt · 2020-06-07T01:46:24Z

GitHubReleases.ps1

+        [Parameter(Mandatory)]
+        [ValidateScript({if (Test-Path -Path $_ -PathType Leaf) { $true } else { throw "$_ does not exist or is inaccessible." }})]
+        [string] $Path,
+
+        [string] $Label,
+
+        [string] $ContentType,


Many releases have multiple assets. Given that this is a separate function, this could maybe implement ValueByPipelineProperty to allow sets of assets to be piped in

rjmholt · 2020-06-07T01:51:06Z

Tests/GitHubReleases.tests.ps1

+            $release = New-GitHubRelease -Uri $repo.svn_url -TagName $script:defaultTagName
+            $queried = Get-GitHubRelease -Uri $repo.svn_url -Release $release.id


I think especially with the release of Pester 5, BeforeAll and AfterAll are highly recommended, rather than embedding setup and teardown directly in the testing block

Pester 5 support is being tracked by #194. Definitely desiring to have that support added. I haven't yet looked into what's involved for that though.

I agree with you that this setup work belongs in BeforeAll/AfterAll. At the time, I forgot about those and specifically wanted to avoid BeforeEach/AfterEach due to the added time from unnecessary duplicated setup that they would incur. Will look into moving setup/teardown as appropriate.

Thanks.

Tests/GitHubReleases.tests.ps1

HowardWolosky · 2020-06-08T00:07:19Z

@rjmholt - Thanks for all the feedback on this. Much appreciated!

GitHubReleases.ps1

HowardWolosky · 2020-07-09T20:57:26Z