Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

API Coverage: Add Support for Deployment Environments #395

Merged
merged 11 commits into from
Apr 30, 2023
Merged

API Coverage: Add Support for Deployment Environments #395

merged 11 commits into from
Apr 30, 2023

Conversation

X-Guardian
Copy link
Contributor

@X-Guardian X-Guardian commented Jan 13, 2023 •
edited
Loading

Description

This PR adds the following new cmdlets for managing GitHub Deployment Environments:

  • New-GitHubDeploymentEnvironment
  • Get-GitHubDeploymentEnvironment
  • Set-GitHubDeploymentEnvironment
  • Remove-GitHubDeploymentEnvironment

Issues Fixed

References

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.
  • 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.
  • 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.
  • If desired, ensure your name is added to our Contributors list

@X-Guardian
Copy link
Contributor Author

@HowardWolosky, this PR is ready for review.

HowardWolosky
HowardWolosky requested changes Feb 27, 2023
View reviewed changes
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this addition. Overall this looks solid, but I do have some minor updates for you to consider.

Interesting how for this newer API, GitHub has aligned both the New and Update to be backed by the same REST API. I wonder if that will continue to be the case in the future....?

USAGE.md Outdated Show resolved Hide resolved
Tests/GitHubDeployments.tests.ps1 Outdated Show resolved Hide resolved
GitHubDeployments.ps1 Outdated
{
<#
.SYNOPSIS
Creates a new deployment environment on a GitHub repository.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that you are aliasing this as both New and Set, you may need to update the documentation language to match (e.g. "Creates a new (or updates an existing) ...") (or use the API docs method of "Creates or updates an ..."). Here and elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

GitHubDeployments.ps1 Show resolved Hide resolved
GitHubDeployments.ps1 Outdated Show resolved Hide resolved
GitHubDeployments.ps1 Outdated
[ValidateSet('ProtectedBranches', 'CustomBranchPolicies', 'None')]
[string] $DeploymentBranchPolicy,

[array] $ReviewerTeamId,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a situation where the module is providing the user back an object where the Team or User ID is a string? (Legitimate question here). If not, it might make more sense to be explicit with the type expectations here as [int32[]].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the types to int64[] to match what is specified in the GitHubRepositories and GutHubTeams functions.

GitHubDeployments.ps1 Show resolved Hide resolved
GitHubDeployments.ps1 Outdated Show resolved Hide resolved
GitHubDeployments.ps1 Outdated
Comment on lines 438 to 442
[Parameter(
ValueFromPipelineByPropertyName,
ParameterSetName='Organization')]
[string] $OrganizationName,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be here?

Suggested change
[Parameter(
ValueFromPipelineByPropertyName,
ParameterSetName='Organization')]
[string] $OrganizationName,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, removed.

GitHubDeployments.ps1 Show resolved Hide resolved
@X-Guardian @HowardWolosky
Apply suggestions from code review
919a10c 
Co-authored-by: Howard Wolosky <HowardWolosky@users.noreply.github.com>
@X-Guardian
Copy link
Contributor Author

@HowardWolosky, any timescale on when you will be able to re-review this PR?

@X-Guardian
Copy link
Contributor Author

@HowardWolosky, any update on the review for this PR?

HowardWolosky
HowardWolosky previously approved these changes Apr 29, 2023
View reviewed changes
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good now except for a few minor nits (three formatting and one typo) which I've added as suggestions so that I can just apply the fixes myself without inconveniencing you further.

GitHubDeployments.ps1 Outdated Show resolved Hide resolved
GitHubDeployments.ps1 Outdated Show resolved Hide resolved
GitHubDeployments.ps1 Outdated Show resolved Hide resolved
GitHubDeployments.ps1 Outdated Show resolved Hide resolved
@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky HowardWolosky merged commit dd844e5 into microsoft:master Apr 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HowardWolosky HowardWolosky HowardWolosky approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

API Coverage: Add Support for Deployment Environments
2 participants
@X-Guardian @HowardWolosky