Add New-GitHubPullRequest command #111

rjmholt · 2019-05-21T06:09:41Z

Resolves #30.

Adds a command for creating new pull requests.

Outstanding questions:

Are the parameters ok, should they be changed?
Are more examples needed?
How do test?

HowardWolosky

This is great! Thanks so much for doing this.
Regarding testing, I'm not quite sure what the best approach is.

I think we could have a test that:

Creates a fork of PowerShellForGitHub to $script:organizationName, and then create a fork of that one to $script:ownerName. It could then do a pull request of master from $script:ownerName's to
$script:organizationName's. I just don't know what the API returns if a PR is created where there are no differences between head and base...

GitHubPullRequests.ps1

HowardWolosky · 2019-05-22T02:37:13Z

GitHubPullRequests.ps1

+        'base' = $Base
+    }
+
+    if ($Body)


Same here:

Suggested change

if ($Body)

if (-not [String]::IsNullOrWhitespace($Body))

I just thought about this again. It's possible to open a PR that has only whitespace in the body I think.

The current implementation doesn't put a restriction on $Title.

So the possible approaches are:

Allow whitespace as $Title or $Body

Silently drop $Body when it's whitespace and throw when $Title is whitespace

Throw when $Body or $Title is whitespacet

GitHubPullRequests.ps1

Co-Authored-By: Howard Wolosky <[email protected]>

HowardWolosky

Great update! Thanks for adding the additional parameterset.
Super minor feedback remaining (new comment in this iteration, as well as the [String]::IsNulllOrWhiteSpace comments from the previous iteration.

GitHubPullRequests.ps1

HowardWolosky

Thanks again for these updates. Your analysis of the Title/Body situation make sense.

In general my philosophy is: If it's something that the API would permit, I don't want to prevent that. If it's something that we know we can catch/validate beforehand, we should since it'll save users time.

One minor change request given a change you made to another part of the file. In general I prefer tests to accompany any new feature. For this one, since it most likely requires a committed change a fork, I'm ok to not have the test. Just let me know if you're planning on writing a test to go with the code before I merge this in once you make the other fix.

Thanks again for the contribution!

HowardWolosky · 2019-05-23T21:57:59Z

GitHubPullRequests.ps1

@@ -117,7 +117,7 @@ function Get-GitHubPullRequest

    $uriFragment = "/repos/$OwnerName/$RepositoryName/pulls"
    $description = "Getting pull requests for $RepositoryName"
-    if (-not [String]::IsNullOrEmpty($PullRequest))
+    if (-not [String]::IsNullOrWhiteSpace($PullRequest))


Hmmmm....if you're making this change here, we should probably be changing $PullRequest from a [string] to [Int] (or [Int64]) since it should really be a number and not a string.

HowardWolosky · 2019-05-28T21:57:17Z

Hey @rjmholt -- just checking back in with you after the long weekend. I'm just looking to see if you were intending to iterate any further on this, or if I should make the updates to this myself after merging in your change.

Thanks again for this contribution!

rjmholt · 2019-05-28T23:29:54Z

Hey @rjmholt Robert Holt FTE -- just checking back in with you after the long weekend. I'm just looking to see if you were intending to iterate any further on this, or if I should make the updates to this myself after merging in your change.

Hi sorry! Yes will continue working on this. Going to add a test this week -- ideally one for each set. I'll also revert that unrelated change in the other function

rjmholt · 2019-05-29T00:36:05Z

I've found and fixed a bug, but when I try to open a PR from this master from my master it gives me a 404 (I'm pretty sure -- I tried it a few different ways).

If I could guarantee other branches it might be possible

PR microsoft#111 adds support for creating new pull requests, but the creator has indicated that there may be a problem with it. Using this to see if I can reproduce the bug.

HowardWolosky · 2019-06-20T15:11:52Z

I've found and fixed a bug, but when I try to open a PR from this master from my master it gives me a 404 (I'm pretty sure -- I tried it a few different ways).

If I could guarantee other branches it might be possible

@rjmholt - Apologies, I missed your previous comment until now. I just tried two PR's using your current submitted code and it works fine for me. Can you clarify the scenario that's giving you the 404? I got a 422 if I tried to submit a PR for a branch that I already had an open PR for, but was unable to reproduce the 404 error you're referring to.

HowardWolosky · 2019-07-26T04:32:27Z

Hey @rjmholt -- just wanted to check-in with you on this one.

rjmholt · 2019-07-30T02:31:25Z

It looks like this for me:

 > New-GitHubPullRequest -OwnerName rjmholt -RepositoryName vscode-PowerShell -Title 'Banana' -Body 'Hi' -Head master -Base master
DONE - Operation took 2 second(s) [Creating pull request Banana in vscode-PowerShell]
Telemetry is currently enabled.  It can be disabled by calling "Set-GitHubConfiguration -DisableTelemetry". Refer to USAGE.md#telemetry for more information. Stop seeing this message in the future by calling "Set-GitHubConfiguration -SuppressTelemetryReminder".
{"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"No commits between master and master"}],"documentation_url":"https://developer.github.com./v3/pulls/#create-a-pull-request"}
At /Users/rjmholt/Documents/Dev/Microsoft/PowerShellForGitHub/GitHubCore.ps1:314 char:17
+                 throw $remoteErrors[0].Exception
+                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : InvalidOperation: (Method: POST, Reque…n; charset=UTF-8
}:HttpRequestMessage) [Invoke-WebRequest], HttpResponseException
+ FullyQualifiedErrorId : WebCmdletWebResponseException,Microsoft.PowerShell.Commands.InvokeWebRequestCommand

Unless there's two branches that exist that we can guarantee commits between, I'm not sure what the best way to proceed with tests is (also just constantly creating PRs will drive the issue number up pretty heavily).

HowardWolosky

Hey -- I think at this point we abandon the idea of the Pester test with this one given the problems you're encountering.
You've done a fantastic job here, and I'd like to just merge in the changes at this point.
Can you just remove the trailing spaces that you have in your change? Once that's done, I'll get this merged in.

Thanks again, so much, for this contribution!

HowardWolosky · 2019-07-30T18:13:13Z

GitHubPullRequests.ps1

+    <#
+    .SYNOPSIS
+        Create a new pull request in the specified repository.
+


nit: can you remove the trailing spaces from the empty lines?

rjmholt · 2019-07-31T16:15:57Z

Hey -- I think at this point we abandon the idea of the Pester test with this one given the problems you're encountering.

Yeah -- sorry I wasn't able to get good tests working. It might be possible to mock Invoke-GHRestMethod to just verify the produced request body (at least that tests the logic of the wrapping cmdlet), but not sure how valuable that would be for the work.

Thanks again, so much, for this contribution!

No worries, sorry it took so long

HowardWolosky

Awesome. Thanks again for the new feature.

HowardWolosky · 2019-07-31T17:45:10Z

/azp run PowerShellForGitHub-CI

azure-pipelines · 2019-07-31T17:45:22Z

Azure Pipelines successfully started running 1 pipeline(s).

HowardWolosky · 2020-06-16T23:13:59Z

@rjmholt - I was reviewing this code today, and I can't find anywhere in the documentation that talks about creating an issue from a pull request:

PowerShellForGitHub/GitHubPullRequests.ps1

Lines 357 to 361 in 788465f

    
           else 
        
           { 
        
               $description = "Creating pull request for issue $Issue in $RepositoryName" 
        
               $postBody['issue'] = $Issue 
        
           }

Is this something that they've removed from the API since you added it? Any idea where you got this idea from? Thanks

Robert Holt and others added 3 commits May 20, 2019 18:03

Add preliminary New-GitHubPullRequest command

bab43cf

Add documentation

1cb9447

Align parameter names with other command

ace9c50

rjmholt marked this pull request as ready for review May 22, 2019 02:26

HowardWolosky requested changes May 22, 2019

View reviewed changes

rjmholt and others added 4 commits May 21, 2019 21:26

Update GitHubPullRequests.ps1

51056ed

Co-Authored-By: Howard Wolosky <[email protected]>

Update GitHubPullRequests.ps1

eaac1be

Co-Authored-By: Howard Wolosky <[email protected]>

Add function export

d3dc7a0

Add issue parameter sets

83e6bf2

HowardWolosky requested changes May 22, 2019

View reviewed changes

GitHubPullRequests.ps1 Show resolved Hide resolved

rjmholt added 3 commits May 22, 2019 13:47

Comment based-help, string::nullorwhitespace

3fcf4d5

Fix body parameter

555dbc6

Add validation for title

92a9e7f

HowardWolosky requested changes May 23, 2019

View reviewed changes

Robert Holt added 2 commits May 28, 2019 16:32

Revert unrelated change

d527931

Fix typo

4156a19

rjmholt changed the title ~~WIP: Add New-GitHubPullRequest command~~ Add New-GitHubPullRequest command Jun 19, 2019

Merge branch 'master' into new-pr-cmdlet

4365fb4

Merge branch 'master' into new-pr-cmdlet

5ebc70c

HowardWolosky requested changes Jul 30, 2019

View reviewed changes

Remove trailing whitespace

ea35eb9

HowardWolosky approved these changes Jul 31, 2019

View reviewed changes

Merge branch 'master' into new-pr-cmdlet

c66f89e

HowardWolosky merged commit 788465f into microsoft:master Sep 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add New-GitHubPullRequest command #111

Add New-GitHubPullRequest command #111

rjmholt commented May 21, 2019

HowardWolosky left a comment

HowardWolosky May 22, 2019

rjmholt May 22, 2019

HowardWolosky left a comment

HowardWolosky left a comment

HowardWolosky May 23, 2019

HowardWolosky commented May 28, 2019

rjmholt commented May 28, 2019

rjmholt commented May 29, 2019

HowardWolosky commented Jun 20, 2019 •

edited

Loading

HowardWolosky commented Jul 26, 2019

rjmholt commented Jul 30, 2019

HowardWolosky left a comment

HowardWolosky Jul 30, 2019

rjmholt commented Jul 31, 2019

HowardWolosky left a comment

HowardWolosky commented Jul 31, 2019

azure-pipelines bot commented Jul 31, 2019

HowardWolosky commented Jun 16, 2020

Add New-GitHubPullRequest command #111

Add New-GitHubPullRequest command #111

Conversation

rjmholt commented May 21, 2019

HowardWolosky left a comment

Choose a reason for hiding this comment

HowardWolosky May 22, 2019

Choose a reason for hiding this comment

rjmholt May 22, 2019

Choose a reason for hiding this comment

HowardWolosky left a comment

Choose a reason for hiding this comment

HowardWolosky left a comment

Choose a reason for hiding this comment

HowardWolosky May 23, 2019

Choose a reason for hiding this comment

HowardWolosky commented May 28, 2019

rjmholt commented May 28, 2019

rjmholt commented May 29, 2019

HowardWolosky commented Jun 20, 2019 • edited Loading

HowardWolosky commented Jul 26, 2019

rjmholt commented Jul 30, 2019

HowardWolosky left a comment

Choose a reason for hiding this comment

HowardWolosky Jul 30, 2019

Choose a reason for hiding this comment

rjmholt commented Jul 31, 2019

HowardWolosky left a comment

Choose a reason for hiding this comment

HowardWolosky commented Jul 31, 2019

azure-pipelines bot commented Jul 31, 2019

HowardWolosky commented Jun 16, 2020

HowardWolosky commented Jun 20, 2019 •

edited

Loading