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.

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

GitHubBranches: Add Get/New/Remove GitHub Repository Branch Pattern Protection Rule #313

Merged

Conversation

X-Guardian
Copy link
Contributor

@X-Guardian X-Guardian commented Jan 24, 2021

Description

This PR adds the following functions to the GitHubBranches module:

  • Invoke-GHGraphQl
  • Get-GitHubRepositoryBranchPatternProtectionRule
  • New-GitHubRepositoryBranchPatternProtectionRule
  • Remove-GitHubRepositoryBranchPatternProtectionRule

Invoke-GHGraphQl is based on Invoke-GHRestMethod but modified with the following features:

  • Use of the GitHub GraphQL API Endpoint.
  • PowerShell 5 and 6+ Invoke-WebRequest error handling support.
  • Use of ThrowTerminatingError instead of Throw for enhanced exceptions and hiding the internal details of the function. Reference: Everything you wanted to know about exceptions
  • GraphQl specific error handling.
  • Extensive debug logging.

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

@HowardWolosky
Copy link
Member

Thanks for this submission, @X-Guardian! I'm really excited to start looking at the code this weekend, as I had wanted to try out the Graph API's but have yet to have a chance.

@HowardWolosky
Copy link
Member

Just a quick update -- I've been slammed with my regular work so I've been delayed with getting this a code review. I'm hoping to have a first-pass review done by the end of this coming weekend if not sooner. I do appreciate your patience here. I'm really looking forward to reading through this code.

@HowardWolosky
Copy link
Member

Just an update here...I'm about halfway through the code review. Only minor issues have been found so far. Hoping to find more time during the week this week to complete it. Thanks again for your patience, and more importantly, for tackling this task!

@X-Guardian
Copy link
Contributor Author

Hi @HowardWolosky, any idea when you might be able to finish the code review on this?

@HowardWolosky
Copy link
Member

@X-Guardian - My sincere apologies here. Life and my main work have kept me attending to this. I'm setting aside actual time this week to work through the remainder of the PR. Again, sincere apologies here as you've clearly put a lot of work into this change, and it's setting up the project for future desired features. The time to review this PR does not at all reflect what I'd prefer the average review time to be.

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.

Phew. Finally managed to get through the whole thing.
Major kudos to you for adding in GraphQl support...that's no easy feat. Even after reviewing all of this code and double-checking it with the documentation, I find GraphQL much less straightforward than the REST API.

Overall, I thought that you did a fantastic job with this. There are a smatter of formatting nits in this feedback, along with a variety of smaller and larger architectural feedback items. I can promise a quicker turnaround on subsequent PR updates, especially now that I got through this first round.

Thanks again for this contribution. It really is valued and appreciated.

GitHubGraphQl.ps1 Outdated Show resolved Hide resolved
GitHubGraphQl.ps1 Show resolved Hide resolved
GitHubBranches.ps1 Outdated Show resolved Hide resolved
GitHubBranches.ps1 Outdated Show resolved Hide resolved
GitHubBranches.ps1 Outdated Show resolved Hide resolved
Tests/GitHubGraphQl.Tests.ps1 Show resolved Hide resolved
Tests/GitHubGraphQl.Tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubGraphQl.Tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubGraphQl.Tests.ps1 Outdated Show resolved Hide resolved
GitHubBranches.ps1 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
Copy link
Member

/azp run PowerShellForGitHub-CI

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

All tests now passing. Good to go.

@HowardWolosky HowardWolosky merged commit f7efc4a into microsoft:master Jul 15, 2021
@JustinGrote
Copy link

Just curious, is there a reason this didn't use the octokit v4 API?
https://github.com/octokit/octokit.graphql.net

@HowardWolosky
Copy link
Member

Just curious, is there a reason this didn't use the octokit v4 API?
https://github.com/octokit/octokit.graphql.net

Good question. In truth, I was unaware of it. However, looking at it, I'm not sure the trades-offs would have been worth it:

  1. It doesn't look like Octokit supports for GitHub Enterprise the way the rest of this module does (which alone makes this a non-starter).
  2. We'd lose the multi-request progress for larger queries.
  3. We'd lose integrated logging.
  4. The code to build the query dynamically based on the function format of our cmdlets would make the Octokit code quite hard to humanly parse/maintain.
  5. Integrating the result object into the same structure that we use for the rest of the module to properly support pipelining would be non-trivial work.

I could probably list more as well. As an aside, while many PowerShell modules themselves are written in .NET and simply expose PowerShell cmdlets to access that .NET-implemented functionality, this module is currently strictly written in PowerShell. So far, that has worked well for us.

I appreciate the heads-up though. It certainly does seem worthwhile to keep an open-mind on this going forward. But, like I said, I don't think using that API really buys us a lot relative to what we'd lose.

@JustinGrote
Copy link

Oh don't get me wrong, I was just curious what the decision criteria was, your graphql implementation is great quality!

@HowardWolosky
Copy link
Member

Oh don't get me wrong ... your graphql implementation is great quality!

Those kudos land squarely on the shoulders of @X-Guardian.

@X-Guardian
Copy link
Contributor Author

Hi @HowardWolosky, it is now coming up to a year since this PR was merged, and there has been no new PowerShellForGitHub releases since then. What is the status of getting a new release? I would like to make additional contributions to this module, but don't see any point if they never get released.

@X-Guardian X-Guardian deleted the GitHubBranches-BranchPattern branch June 15, 2022 08:30
@HowardWolosky
Copy link
Member

I realize that the delay has been frustrating -- it's been frustrating for me as well. I'm still navigating internal issues with getting the necessary steps in plan to be able to re-enable signing. That's what all the delays come down to. I can't release further updates under the microsoft account if the module files aren't signed, and the signing process internally underwent a huge overhaul last year without consideration of how it affects open-source projects like this which aren't directly associated with a larger product.

I will post updates as I have them, and I'm optimistic that I'll be able to make further progress to get signing re-enabled.

@HowardWolosky
Copy link
Member

Good news! I believe I have a workaround for my signing issues and will be able to release a signed version of 0.16.1 next week. That workaround should also allow me to continue to release signed versions going forward as well, although it will have to be done manually as opposed to through the Release pipeline.

@HowardWolosky
Copy link
Member

0.16.1 has been successfully signed and published to PowerShellGallery. This proves out the new approach I need to use to sign releases moving forward. It's now a manual approach (can't use an Azure Pipeline anymore), but it at least gets things unblocked for the project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants