Add Visibility parameter to [New|Set]-GitHubRepository

[mkellerman]

Description

Add Visibility parameter to New-GitHubRepository & Set-GitHubRepository

Issues Fixed

References

https://developer.github.com/v3/repos/#create-a-repository-for-the-authenticated-user https://developer.github.com/v3/repos/#update-a-repository

Checklist

• [X] You actually ran the code that you just wrote, especially if you did just "one last quick change".
• [X] Comment-based help added/updated, including examples.
• [X] Static analysis is reporting back clean.
• [X] New/changed code adheres to our coding guidelines.
• [ ] Formatters were created for any new types being added.
• [ ] New/changed code continues to support the pipeline.
• [ ] Changes to the manifest file follow the manifest guidance.
• [X] Unit tests were added/updated and are all passing. See testing guidelines. This includes making sure that all pipeline input variations have been covered.
• [ ] Relevant usage examples have been added/updated in USAGE.md.
• [X] If desired, ensure your name is added to our Contributors list

[ghost]

All CLA requirements met.

[HowardWolosky]

I also think that we can a little better than the API does in this scenario (like when they call -Private -Visibility Public). We should either prevent users from making this mistake by erroring out, or at least specify a warning. If I had my way, we'd just deprecate -Private entirely and move to using -Visibility solely to prevent those issues, but 0.15.0 already introduced a host of breaking changes and I'm a bit reluctant to introduce more right now.

For the time being, can you add a warning if both parameters have been specified and they indicate different values?

    if ($PSBoundParameters.ContainsKey('Visibility') -and
        $PSBoundParameters.ContainsKey('Private') -and
        (($Private -and ($Visiblity -ne 'Private')) -or
         ((-not $Private) -and ($Visibility -ne 'Public'))))
    {
        Write-Log -Level Warning 'The value specified by Visibility will override the value specified by Private when both are specified.'
    }

[mkellerman]

Added tests and warnings. Let me know what you think.

[HowardWolosky]

/azp run PowerShellForGitHub-CI

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[HowardWolosky]

@mkellerman - Looks like your new tests are reliably failing (both tests failed on all 4 jobs). Can you take a look?

[mkellerman]

@mkellerman - Looks like your new tests are reliably failing (both tests failed on all 4 jobs). Can you take a look?

Yes. Currently trying to setup the pester tests locally on my machine.

[mkellerman]

I bet it doesn't use the -Visibility property unless your using GitHub Enterprise Cloud or GitHub Enterprise Server 2.20+

Is there a way to get the version of the server we are connecting to? Is there a way to test against an Enterprise Server?

The code works when pointing against my GitHub Enterprise, but not GitHub.com.

[HowardWolosky]

I bet it doesn't use the -Visibility property unless your using GitHub Enterprise Cloud or GitHub Enterprise Server 2.20+

Hmmm...I'm seeing the same thing. Admittedly, I should have privately tested your change before approving it, but I had felt good with the code inspection and relying on the result of the UT's (which at least caught this properly).

I feel like their documentation on this point is poor, or they have a bug...it's not clear which it is. Either they need to clarify in the documentation that this parameter is only valid for Enterprise customers, or they need to fix the bug that uses it.

Is there a way to get the version of the server we are connecting to?

Not that I'm aware of (I haven't seen anything like that in the docs, and I just looked again). I assume though that you had to change the value of apiHostName to test against your server?

Is there a way to test against an Enterprise Server?

I don't currently have those resources. I've reached out to them to see if I can get a development license. Even if that comes through though, we need these tests to reliably pass regardless of the environment.

The code works when pointing against my GitHub Enterprise, but not GitHub.com.

Good to know. I recommend opening up a thread on the GitHub Ecosystem forum about this issue. They either need to change their documentation to indicate that Visibility can only be used on Enterprise sku's, or they have a bug that isn't applying its value of public or private when used on GitHub.com, and then reply back with the link for us to track.

We'll just leave this PR open for the time being as I'm not quite sure how to proceed until we have a response. If it's a product bug, we'll wait for the bug to be fixed before merging this in, in order to avoid letting users get into an unexpected repo state. If it's a documentation issue, then we can reassess at that point how to best handle it. It might be that we are smart under the covers and we just convert Visibility to the appropriate value for Private when the configured apiHostName is not github.com, and throw an error in the scenario where the configured hostname is github.com but the user specifies Internal for Visibility.

[mkellerman]

We could throw an error if apiHostName is GitHub.com and Visibility is used. All other hostnames should be an Enterprise server, but there is no way to validate this, or the version, and that's a huge assumption.

I will open a thread on GitHub Ecosystem form as recommended.

[HowardWolosky]

I will open a thread on GitHub Ecosystem form as recommended.

Excellent, thanks.

We could throw an error if apiHostName is GitHub.com and Visibility is used. All other hostnames should be an Enterprise server, but there is no way to validate this, or the version, and that's a huge assumption.

Let's see what they say on whether or not this is a bug or a documentation issue. If it's a bug, we probably wait until it's fixed. If it's a documentation issue, then we can throw the error if the apiHostName is github.com as well as if the Visibility was specified and the value in the repository object from the executed request doesn't match the input value. That's iteratively better, but still not ideal.

[mkellerman]

Noted.

Feel free to add any comments or information: https://github.community/t/create-a-repository-on-github-com-using-visibility-parameter/149265

[mkellerman]

I found the blog post about the feature, which seems to say it is only available to GitHub Enterprise accounts.

https://developer.github.com/changes/2019-12-03-internal-visibility-changes/

This might indicate a documentation error.

[HowardWolosky]

Still no official response from GitHub on this, unfortunately.

I don't think we can make the assumption that Enterprise accounts use a different apiHostName after all.

An alternate approach that could be taken is to have it internally convert a Visibility of Public or Private to the appropriate $Private boolean value, and submit it that way. Then we should be in a better state where we only ever attempt to use Visibility when the value is set to Internal. The downside there is that if they specify Internal and it's not an Enterprise account, the repo will end up as public. That might require a warning with confirmation (possibly using -Force to accept the confirmation silently).

Open to your thoughts here.

[mkellerman]

We could try to execute the Visibility parameter, and get the status back.

If fail, use the Public or Private parameter, and if Internal give an error, as we cannot execute the command as expected.

If I use Visibility, I expect the outcome to do what I told it to do. We should throw an error only when it cannot.

What do you think?

[HowardWolosky]

I believe previous experimentation showed that when Visibility was used with private on a non-Enterprise account, the result was that the repo was made public. Making a private repo public is an irreversible change. Our first priority has to be the protection of user data...

If the user's intent is to set it to public or private, then it shouldn't matter if we internally do it via Visibility or Private, so long as the result is what is expected. It really comes down to how we handle the Internal scenario when that is specified on a non-Enterprise account.

[mkellerman]

Absolutely right. I forgot about that part. Good catch!

Okay, I'll make the changes, and request a review.

[mkellerman]

Should we do something like this:

    if ($PSBoundParameters.ContainsKey('Visibility') -and $PSBoundParameters.ContainsKey('Private') -and
        (($Private -and ($Visiblity -ne 'Private')) -or ((-not $Private) -and ($Visibility -ne 'Public'))))
    {
        Write-Log -Level Warning 'The value specified by Visibility will override the value specified by Private when both are specified.'
    }

    if ($PSBoundParameters.ContainsKey('Visibility')) {
        Switch($Visibility) {
            'Private'  { $Private = [switch]$True }
            'Public'   { $Private = [switch]$False }
            'Internal' { $Private = [switch]$True }
        }
    }

So if the Visibility property is not available in enterprise, it will at least set it to Private? We could later check, to see if it was set to Internal, and throw a warning message, telling the user what happened.

[HowardWolosky]

Have you experimented with what happens on an Enterprise account when both Private and Visibility are set (in the various combinations)? If they do ignore Private, then I think your suggestion will work. You should try it with all the different combinations to be sure:

Private	Visibility
$false	public
$false	private
$false	internal
$true	public
$true	private
$true	internal

[microsoft-github-policy-service[bot]]

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 14 days of this comment.

[microsoft-github-policy-service[bot]]

This pull request has been automatically closed due to a lack of activity from the author. We understand. Life happens and other things likely came up. We would still love to see your contribution get merged in. Now that it has been closed, a different community member may wish to pick up where you left off. If so, they should speak up by commenting below. If you're still interested in completing this yourself, just respond back and let us know.