microsoft / PowerShellForGitHub

Microsoft PowerShell wrapper for GitHub API
Other
588 stars 186 forks source link

Add support for codespaces #407

Closed vercellone closed 1 year ago

vercellone commented 1 year ago

Description

Add support for Codespaces.

Issues Fixed

References

Endpoints excluded below will be added in subsequent PRs

Codespaces/codespaces

Codespaces/organizations

vercellone commented 1 year ago

Added

Keep the feedback coming. I'm particularly interested in your take on INPUTS, -Uri, -RepositoryId, and -PullRequest usage for New.

I will work on Tests next when able.

vercellone commented 1 year ago

Good news Repositories do not require any prep to create a codespace with New-GitHubCodespace. Of my repos, only vercellone/my-first-codespace includes .devcontainer/ config, and I can create viable codepaces for any of my repos. Given that, testing looks pretty straightforward.

Bad news The url returned is always of the form https://api.github.com/user/codespaces/{codespace_name}, with no owner nor repo elements. Therefore, URI + Resolve-RepositoryElements isn't viable. I can figure something out, but suggestions are appreciated.

HowardWolosky commented 1 year ago

Good news Repositories do not require any prep to create a codespace with New-GitHubCodespace. Of my repos, only vercellone/my-first-codespace includes .devcontainer/ config, and I can create viable codepaces for any of my repos. Given that, testing looks pretty straightforward.

Great!

Bad news The url returned is always of the form https://api.github.com/user/codespaces/{codespace_name}, with no owner nor repo elements. Therefore, URI + Resolve-RepositoryElements isn't viable. I can figure something out, but suggestions are appreciated.

At what point are you encountering this issue? In Add-GitHubCodespacesAdditionalProperties? If so, you have two options there:

  1. You can model something similar to what we do elsewhere where we think we may not have the necessary information in the response object -- just pass in the information we already know as an optional parameter. We do that for Repositories here:

https://github.com/microsoft/PowerShellForGitHub/blob/6f94a9b0a37ee466c2fb457299c78de1bd371f95/GitHubRepositories.ps1#L3673-L3702

  1. The other option is to leverage the fact that a repository object is part of the response object and you're already sending it into Add-GitHubRepositoryAdditionalProperties, so you can just grab RepositoryUrl from the modified repository object afterwards and then add it to the root of your new object.

If you're encontering this issue elsewhere, then I'll need more context to know what and where the problem is.

vercellone commented 1 year ago

Bad news The url returned is always of the form [https://api.github.com/user/codespaces/{codespace_name}]

I went with Option 2

vercellone commented 1 year ago

I added some tests for Get-GitHubCodespace. I'm guessing whatever feedback you'll have will likely be applicable to the remaining tests as well. But, I will work on them in the meantime whether I hear back or not.

vercellone commented 1 year ago

Tests added

New-GitHubCodespace tests are still forthcoming. As are Remove-GitHubCodespace and Stop-GitHubCodespace tests for Organization parameter sets.

A -Force switch was added to Remove-GitHubCodespace, but the current ConfirmImpact is the default of Medium. Do you think it should be set to High, or should the -Force switch be dropped? The codespace is easily rebuilt but does maintain the state of work in progress. I'm leaning toward medium with no Force, but could be easily swayed.

HowardWolosky commented 1 year ago

I really appreciate all the effort you're putting into this support. Thanks so much!

A -Force switch was added to Remove-GitHubCodespace, but the current ConfirmImpact is the default of Medium. Do you think it should be set to High, or should the -Force switch be dropped? The codespace is easily rebuilt but does maintain the state of work in progress. I'm leaning toward medium with no Force, but could be easily swayed.

I feel like it would be best if the module was consistent with how it handles all Remove-* methods. A consistent interface helps users feel comfortable and safe when exploring other parts of a module. In this case, it should keep the -Force switch, and have the same behavior that the other Remove-* methods have when it comes to checking -Force and $Confirm, as well as $PSCmdlet.ShouldProcess. Hope that helps.

BTW: I won't be looking at the updated PR again until you explicitly request it.

vercellone commented 1 year ago

I feel like it would be best if the module was consistent with how it handles all Remove-* methods.

Agreed. ConfirmImpact set to High.

BTW: I won't be looking at the updated PR again until you explicitly request it.

In that case I'll contribute functions for the remaining endpoints in a separate PR. I've excluded them above, accordingly. Once I get the outstanding tests and usage committed I'll let you know.

vercellone commented 1 year ago

Tests pushed. I still owe you Usage, but please review if able.

vercellone commented 1 year ago

Usage committed. Please review @HowardWolosky.

HowardWolosky commented 1 year ago

Usage committed. Please review @HowardWolosky.

Noted, thanks! Will take a look this weekend.

vercellone commented 1 year ago

Thanks so much for all of this work. SUPER appreciated. Sincere apologies as well on the delayed review. Life happened and I couldn't give this the dedicated time it needed prior to today.

Np. Life is like that sometimes.

There's a lot of feedback here, but none of it is super large, so I don't think it will take too long to apply any of it (aside from some of the questions I have in some of the tests around the super-fake-test-org).

Good feedback. There are 4 unresolved conversations to which I replied but committed no related changes. Of those only the "other parameters...dynamically referenced via the propertyMap" demands changes. I'm hoping you can elaborate on that one.

vercellone commented 1 year ago

@HowardWolosky Are you waiting on me for anything? What is the next step?

vercellone commented 1 year ago

@HowardWolosky?

HowardWolosky commented 1 year ago

Ack -- sorry about that. Other things came up and I missed your previous ping on this. I have this queued to look at this weekend.

HowardWolosky commented 1 year ago

Ack -- sorry about that. Other things came up and I missed your previous ping on this. I have this queued to look at this weekend.

Sincerest apologies here. Clearly last weekend didn't happen. I don't want to make another promise that can't be kept, so I'll simply promise that I'll complete my review of the last iteration by the end of next weekend. If any additional updates are required coming out of that, I'll promise a 48 hour review turnaround on those. I greatly appreciate your contribution and want you to feel like it is valued (because it is)...I'm just trying to balance a number of different obligations here.

vercellone commented 1 year ago

Thanks for your patience on this updated review. I've added some new feedback for you to consider, and re-activated some previous comments that had been resolved without any corresponding change (or alternatively a corresponding comment explaining why there was no change in response).

I'll run the tests locally after the next update to make sure that nothing was missed.

Thanks again for this contribution (and again, for your patience)!

Back to you @HowardWolosky.

vercellone commented 1 year ago

2 left

  1. Should we test When getting all codespaces for a specified organization? If so, how?
  2. Should we drop the location assertion? I vote no.

Let me know.

vercellone commented 1 year ago
Invoke-Pester -Configuration (
    [PesterConfiguration]@{
        Filter = @{ FullName = 'GitHubCodespaces\*-GitHubCodespace' }
        Output = @{ Verbosity = 'Detailed' }
        Path   = 'C:\Users\Jason\source\GitHub\PowerShellForGitHub\Tests\GitHubCodespaces.tests.ps1'
    }
)

Pester v5.4.1

Starting discovery in 28 files.
Discovery found 1128 tests in 616ms.
Filter 'FullName' set to ('GitHubCodespaces\*-GitHubCodespace').
Filters selected 30 tests to run.
Running tests.

Running tests from 'C:\Users\Jason\source\GitHub\PowerShellForGitHub\Tests\GitHubCodespaces.tests.ps1'
Describing GitHubCodespaces\Delete-GitHubCodespace
 Context When deleting a codespace for the authenticated user
   [+] Should get no content using -Confirm:$false 15.06s (15.06s|2ms)
   [+] Should get no content using -Force 15.02s (15.02s|1ms)

Describing GitHubCodespaces\Get-GitHubCodespace
 Context When getting codespaces for the authenticated user
   [+] Should return objects of the correct type 3ms (2ms|2ms)
   [+] Should return one or more results 3ms (2ms|1ms)
   [+] Should return the correct properties 2ms (2ms|1ms)
 Context When getting a codespace for a specified owner and repository
   [+] Should return objects of the correct type 3ms (2ms|1ms)
   [+] Should return one or more results 3ms (2ms|1ms)
   [+] Should return the correct properties 3ms (2ms|1ms)
 Context When getting a codespace for a specified organization user
   [+] Should have results for the organization user 4ms (3ms|1ms)
   [+] Should return the correct properties 3ms (2ms|1ms)
 Context When getting a codespace for a specified codespace name
   [+] Should return objects of the correct type 3ms (2ms|1ms)
   [+] Should return the correct properties 2ms (2ms|1ms)
 Context When specifiying the Uri parameter
   [+] Should return objects of the correct type 3ms (2ms|1ms)
   [+] Should return the correct properties 3ms (2ms|1ms)
 Context When specifiying the Uri parameter from the pipeline
   [+] Should return objects of the correct type 4ms (2ms|1ms)
   [+] Should return the correct properties 2ms (2ms|1ms)

Describing GitHubCodespaces\New-GitHubCodespace
 Context When creating a repository for the authenticated user
  Context When creating a codespace with default settings with RepositoryId
    [+] Should return an object of the correct type 10ms (6ms|3ms)
    [+] Should return the correct properties 4ms (3ms|1ms)
  Context When creating a codespace with default settings with Ref
    [+] Should return an object of the correct type 4ms (2ms|2ms)
    [+] Should return the correct properties 4ms (3ms|1ms)
  Context When creating a codespace with default settings from a PullRequest
    [+] Should return an object of the correct type 3ms (2ms|1ms)
    [+] Should return the correct properties 4ms (3ms|1ms)
  Context When creating a codespace with all possible settings
    [+] Should return an object of the correct type 3ms (2ms|1ms)
    [+] Should return the correct properties 7ms (7ms|1ms)
  Context When creating a codespace with default settings with Repository Elements
    [+] Should return an object of the correct type 3ms (2ms|1ms)
    [+] Should return the correct properties 4ms (3ms|1ms)

Describing GitHubCodespaces\Start-GitHubCodespace
 Context When starting a codespace for the authenticated user
   [+] Should not throw 4.26s (4.25s|2ms)
   [+] Should become Available 7.48s (7.48s|1ms)

Describing GitHubCodespaces\Stop-GitHubCodespace
 Context When stopping a codespace for the authenticated user
   [+] Should not throw 3.9s (3.9s|3ms)
   [+] Should become Shutdown 6.78s (6.78s|1ms)
Tests completed in 206.51s
Tests Passed: 30, Failed: 0, Skipped: 0 NotRun: 1098
vercellone commented 1 year ago

Still seeing two test failures:

Thanks for the quick responses this week.

I could not reproduce these last 2 test failures in pwsh 7.3.6, but I was able to reproduce them in Windows PowerShell 5.1. They require an @ symbol before the ($codespaces...).Count to force the result to an Object[] array and avoid the InvokeMethodOnNull exception.

I think we should be good now.

HowardWolosky commented 1 year ago

Can you also add last_used_at to this (will ensure that it gets converted to a real date/time object in the return object by default)?

https://github.com/microsoft/PowerShellForGitHub/blob/482fe2331732a24ed779b140c3f38c8a156271e1/GitHubCore.ps1#L990-L1008

vercellone commented 1 year ago

last_used_at added to datePropertyNames

vercellone commented 1 year ago