Skip to content

New GitHubRepositoryBranchProtectionRule Functions #238

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
Closed
wants to merge 7 commits into from
Closed

New GitHubRepositoryBranchProtectionRule Functions #238

wants to merge 7 commits into from

Conversation

X-Guardian
Copy link
Contributor

@X-Guardian X-Guardian commented Jun 14, 2020 •
edited by HowardWolosky
Loading

This PR is superseded by PR #255

Description

This PR adds the following GitHub Branch Protection Rule functions to the GitHubBranches module:

  • Get-GitHubRepositoryBranchProtectionRule
  • Set-GitHubRepositoryBranchProtectionRule
  • Remove-GitHubRepositoryBranchProtectionRule

The lukeCageAcceptHeader was also added to the GitHubCore module with a value of application/vnd.github.luke-cage-preview+json.

Issues Fixed

None

References

GitHub REST API v3

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

@X-Guardian
Copy link
Contributor Author

Can we trigger the CI for this PR?

@HowardWolosky
Copy link
Contributor

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@X-Guardian X-Guardian marked this pull request as ready for review June 14, 2020 18:37
@HowardWolosky HowardWolosky added enhancement An issue or pull request introducing new functionality to the project. api-repositories-branches Work to complete the API's defined here: https://developer.github.com/v3/repos/branches/ api completeness This is basic API functionality that hasn't been implemented yet. and removed enhancement An issue or pull request introducing new functionality to the project. labels Jun 18, 2020
HowardWolosky
HowardWolosky suggested changes Jun 20, 2020
View reviewed changes
Copy link
Contributor

@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.

Kudos for your work on trying to adapt this very complicated API into a function.
In addition to the needed work to adapt for pipeline support, I have a number of pieces of feedback for you to consider before this can get merged in.

Thanks again for all your effort!

GitHubBranches.ps1
$RestrictPushTeams = @()
}

$restrictions = @{
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to my feedback above regarding required_status_checks, this will introduce regressions if users try to update just the push users or just the teams. If I already have values defined for users, and now I go and want to make a separate call to set the teams, this code, as written, will wipe out my defined users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GitHub have really over-complicated this API and it doesn't allow you to do this. To achieve this functionality, all the other individual API endpoints would need to be implemented. I was originally thinking of making this function New-GitHubRepositoryBranchProtectionRule and am happy to go back to that if required.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow. You've already implemented Get-GitHubRepositoryBranchProtectionRule. If you detected that any of these object properties were being altered and called that first, wouldn't you have all the content that you'd need in order to ensure you weren't overwriting any property that the user didn't provide in this call?

@HowardWolosky
Copy link
Contributor

@X-Guardian - please be sure to @ mention me once this is ready for the next review (there are so many of your reviews for me to keep track of). Thanks again for all your work. Much appreciated!

@HowardWolosky HowardWolosky added the waiting for update Waiting for an update to the PR before the next review label Jun 20, 2020
HowardWolosky
HowardWolosky reviewed Jun 21, 2020
View reviewed changes
GitHubBranches.ps1
Specify which users have push access.

.PARAMETER RestrictPushTeams
Specify which teams have push access.
Copy link
Contributor

Choose a reason for hiding this comment

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

And actually, saying that and then reviewing the code, it occurs to me that you don't have OrganizationName as a parameter, which means that users can't specify team names that are owned by an organization, which could be a problem if the repo that the rule is being applied to is one owned by an organization.

HowardWolosky
HowardWolosky suggested changes Jun 22, 2020
View reviewed changes
Copy link
Contributor

@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.

Looks like there's still some iteration to go through, between these new comments and the existing unresolved threads we have going on.

Again though, I really appreciate your efforts here.

GitHubBranches.ps1
$RestrictPushTeams = @()
}

$restrictions = @{
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow. You've already implemented Get-GitHubRepositoryBranchProtectionRule. If you detected that any of these object properties were being altered and called that first, wouldn't you have all the content that you'd need in order to ensure you weren't overwriting any property that the user didn't provide in this call?

GitHubBranches.ps1

if ($PSBoundParameters.ContainsKey('StatusChecks'))
{
$requiredStatusChecks = @{
Copy link
Contributor

Choose a reason for hiding this comment

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

I would define $requiredStatusChecks = $null outside of the if, and then you won't need the else at all.
We don't currently have strict mode on right now, but I still prefer to code that way. The way you have it right now, you're declaring a variable within the if scope, so while PowerShell allows it to be accessed outside of that scope, it's still not a great practice to follow. You generally want to be declaring/initially using your variables in the scope you're intending to use them, which in thi scase, is outside of the if scope.

@HowardWolosky HowardWolosky mentioned this pull request Jun 24, 2020
Closed
8 tasks
@X-Guardian X-Guardian mentioned this pull request Jun 28, 2020
Closed
9 tasks
@X-Guardian
Copy link
Contributor Author

This PR is superseded by PR #255

@X-Guardian X-Guardian closed this Jun 29, 2020
@HowardWolosky HowardWolosky added superseded The PR was replaced by a newer PR and removed waiting for update Waiting for an update to the PR before the next review labels Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HowardWolosky HowardWolosky HowardWolosky requested changes

Assignees
No one assigned
Labels
api completeness This is basic API functionality that hasn't been implemented yet. api-repositories-branches Work to complete the API's defined here: https://developer.github.com/v3/repos/branches/ superseded The PR was replaced by a newer PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@X-Guardian @HowardWolosky