search

microsoft / PowerShellForGitHub

Microsoft PowerShell wrapper for GitHub API
Other
584 stars 184 forks source link

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

Closed X-Guardian closed 2 years ago

X-Guardian commented 3 years ago

Description

This PR adds the following functions to the GitHubBranches module:

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

Issues Fixed

References

Checklist

HowardWolosky commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

HowardWolosky commented 3 years ago

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

HowardWolosky commented 3 years ago

/azp run PowerShellForGitHub-CI

azure-pipelines[bot] commented 3 years ago
Azure Pipelines successfully started running 1 pipeline(s).
HowardWolosky commented 3 years ago

/azp run PowerShellForGitHub-CI

azure-pipelines[bot] commented 3 years ago
Azure Pipelines successfully started running 1 pipeline(s).
X-Guardian commented 3 years ago

Hi @HowardWolosky, any update on when you are going to be able to look at this again?

HowardWolosky commented 3 years ago

This completely fell off my radar. My apologies. I'll get this queued for another review this week.

X-Guardian commented 3 years ago

Any update on reviewing this PR @HowardWolosky?

HowardWolosky commented 3 years ago

As my weekends have proven challenging to finish this review, I have booked an hour of time on my work calendar this week to complete the final pass of this review. I greatly appreciate your patience here.

X-Guardian commented 3 years ago

Any update on reviewing this PR @HowardWolosky ?

X-Guardian commented 3 years ago

Any update on reviewing this PR @HowardWolosky ?

HowardWolosky commented 2 years ago

Any update on reviewing this PR @HowardWolosky ?

I promise you that as long as there is nothing considerably wrong with the PR at this point, I will have it merged before Monday morning and then personally address any remaining issues that I have with it as post-merge bug fixes that I make myself. You've been waiting too long (albeit patiently) to get this merged in, and you deserve to have your worked merged-in. My sincere apologies.

HowardWolosky commented 2 years ago

/azp run PowerShellForGitHub-CI

azure-pipelines[bot] commented 2 years ago
Azure Pipelines successfully started running 1 pipeline(s).
HowardWolosky commented 2 years ago

There are 3-4 UT's introduced with this change that are failing. I'd like to have these tests passing before being merged in. I'll take a look into fixing them myself in the coming days, or if you are able to, then go for it. I promise to review any update/fix within 24 hours so that we can get this merged in.

X-Guardian commented 2 years ago

@HowardWolosky, I've fixed the two New-GitHubRepositoryBranchPatternProtectionRule and one Invoke-GhGraphQl unit tests that were failing. GitHub had changed their exception message output.

There was a Get-GitHubGist test failing, but that is not applicable to this PR.

HowardWolosky commented 2 years ago

/azp run PowerShellForGitHub-CI

azure-pipelines[bot] commented 2 years ago
Azure Pipelines successfully started running 1 pipeline(s).
HowardWolosky commented 2 years ago

@X-Guardian - Any chance you can fix the last Invoke-GraphQl failure (it's only failing on Mac/Linux)? If not, I can take a look at fixing it tomorrow.

HowardWolosky commented 2 years ago

/azp run PowerShellForGitHub-CI

azure-pipelines[bot] commented 2 years ago
Azure Pipelines successfully started running 1 pipeline(s).
HowardWolosky commented 2 years ago

/azp run PowerShellForGitHub-CI

azure-pipelines[bot] commented 2 years ago
Azure Pipelines successfully started running 1 pipeline(s).
HowardWolosky commented 2 years ago

/azp run PowerShellForGitHub-CI

azure-pipelines[bot] commented 2 years ago
Azure Pipelines successfully started running 1 pipeline(s).
HowardWolosky commented 2 years ago

/azp run PowerShellForGitHub-CI

azure-pipelines[bot] commented 2 years ago
Azure Pipelines successfully started running 1 pipeline(s).
HowardWolosky commented 2 years ago

/azp run PowerShellForGitHub-CI

azure-pipelines[bot] commented 2 years ago
Azure Pipelines successfully started running 1 pipeline(s).
JustinGrote commented 2 years ago

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

HowardWolosky commented 2 years ago

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 commented 2 years ago

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

HowardWolosky commented 2 years ago

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

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

X-Guardian commented 2 years ago

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.

HowardWolosky commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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.