Add support for merge method and repository squash settings. by gsquire · Pull Request #446 · google/go-github

gsquire · 2016-10-10T19:55:28Z

This PR adds merge method support and squash API options for repository get and update.

I'm unsure of what a good unit test might be, so I can update this if need be.

gmlewis · 2016-10-10T20:53:04Z

 // PullRequestOptions lets you define how a pull request will be merged.
 type PullRequestOptions struct {
-	Squash bool
+	Squash      bool


It looks to me like Squash is no longer supported and can be removed..
https://developer.github.com/changes/2016-04-01-squash-api-preview/ seems to confirm, although it is not explicit.

I agree, it looks to be deprecated.

gmlewis · 2016-10-10T20:55:02Z

 type PullRequestOptions struct {
-	Squash bool
+	Squash      bool
+	MergeMethod string


Can you please document this field?
e.g.: Merge method to use. Possible values are: "merge", "squash", "rebase". Default is "merge".

gmlewis · 2016-10-10T20:56:59Z

@@ -251,12 +251,14 @@ type PullRequestMergeResult struct {

 // PullRequestOptions lets you define how a pull request will be merged.
 type PullRequestOptions struct {


Could you please add CommitTitle to this struct?
https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button

Yes, I certainly missed that in the blog post.

gmlewis · 2016-10-10T20:57:35Z


 type pullRequestMergeRequest struct {
 	CommitMessage *string `json:"commit_message"`
 	Squash        *bool   `json:"squash,omitempty"`


You will need to remove Squash here, too.

gmlewis · 2016-10-10T20:57:46Z

+	MergeMethod string
 }

 type pullRequestMergeRequest struct {


You will need to add CommitTitle here too.

gmlewis · 2016-10-10T20:58:12Z

@@ -268,10 +270,12 @@ func (s *PullRequestsService) Merge(owner string, repo string, number int, commi
 	pullRequestBody := &pullRequestMergeRequest{CommitMessage: &commitMessage}
 	if options != nil {


And update this section too.

gmlewis · 2016-10-10T20:59:21Z


 	// TODO: remove custom Accept header when the license support fully launches
 	// https://developer.github.com/v3/licenses/#get-a-repositorys-license
+	// https://developer.github.com/changes/2016-09-26-pull-request-merge-api-update


The blog posts are usually referenced in the github.go file with the preview tags - you could add this to the polaris one.

Sure, will do.

gsquire

Thanks for the CR.

gsquire · 2016-10-10T21:42:02Z

@@ -251,12 +251,14 @@ type PullRequestMergeResult struct {

 // PullRequestOptions lets you define how a pull request will be merged.
 type PullRequestOptions struct {


Yes, I certainly missed that in the blog post.

gsquire · 2016-10-10T21:42:14Z

 // PullRequestOptions lets you define how a pull request will be merged.
 type PullRequestOptions struct {
-	Squash bool
+	Squash      bool


I agree, it looks to be deprecated.

gsquire · 2016-10-10T21:42:39Z


 	// TODO: remove custom Accept header when the license support fully launches
 	// https://developer.github.com/v3/licenses/#get-a-repositorys-license
+	// https://developer.github.com/changes/2016-09-26-pull-request-merge-api-update


Sure, will do.

gmlewis · 2016-10-10T21:55:01Z

-	Squash        *bool   `json:"squash,omitempty"`
+	CommitTitle   *string `json:"commit_title,omitempty"`
+
+	// The merge method to use. Possible values include: "merge", "squash", and "rebase" with the default being merge.


While this is good, I think it would be much better to move this to the exported struct above (PullRequestOptions) so that the auto-generated godocs will read better.

No problem, should have caught that.

gmlewis · 2016-10-10T21:56:42Z

LGTM after a minor godoc tweak.
Awaiting secondary LGTM before merging.

gsquire · 2016-10-10T22:01:53Z

Thank you, I will collapse my commits when it gets signed off on.

gsquire · 2016-10-12T00:46:23Z

I rebased onto master and squashed.

dmitshur · 2016-10-12T07:53:13Z

We should add a "Resolves #437." or equivalent message to the commit message, so that issue gets closed when this is merged. But @gmlewis can do that when merging, so it's not a big deal.

dmitshur

See question about multiple preview API accept headers in inline comments.

dmitshur · 2016-10-12T08:01:39Z

 	// TODO: remove custom Accept header when the license support fully launches
 	// https://developer.github.com/v3/licenses/#get-a-repositorys-license
 	req.Header.Set("Accept", mediaTypeLicensesPreview)
+	req.Header.Add("Accept", mediaTypeSquashPreview)


Has anyone tested that multiple Accept headers actually work by combining both preview APIs, instead of only one being used?

I'm asking because this is the very first time I see it done in this library, it has never been done before as far as I know.

I tried reading https://developer.github.com/v3/media/ and it doesn't seem to say that multiple Accept headers are the way to go (neither does it say it shouldn't be done).

Yeah, pretty sure that 2 accept headers don't work:

$ curl https://api.github.com/users/technoweenie -I -s -H "Accept: application/vnd.github.drax-preview+json" -H "Accept: application/vnd.github.polaris-preview+json" | grep X-GitHub-Media-Type X-GitHub-Media-Type: github.drax-preview; format=json

Only one seems to have an effect.

I'm not sure if it's even possible to opt into two preview APIs in the same request... Any thoughts on this?

I didn't know if that would matter or not. I assumed that you would need both since this API request now has two preview features. What should happen here then?

Sorry for the delay. I will ask GitHub support and see what they recommend.

It's no rush, I didn't know what the course of action was for this. Thanks!

I will ask GitHub support and see what they recommend.

Good idea, thanks for doing that!

If it turns out that it's not possible to combine multiple beta APIs in same request, we might have to make 2 requests (each with a different beta API), or just pick 1 beta that's more important and remove the other. Or wait until at least one of them is merged into main v3 API, then use the remaining beta header.

It turns out that my hunch for using comma-separated values was correct:
https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

From GitHub support:

Example:
Accept: application/vnd.github.mockingbird-preview, application/vnd.github.polaris-preview

Of course, this will need to be tested.

Great, I will update it and do some manual tests and see if the unit test can be changed too.

dmitshur · 2016-10-12T08:01:46Z

 	mediaTypeReactionsPreview = "application/vnd.github.squirrel-girl-preview"

 	// https://developer.github.com/changes/2016-04-01-squash-api-preview/
+	// https://developer.github.com/changes/2016-09-26-pull-request-merge-api-update


Nitpick, but the missing trailing slash is inconsistent.

gsquire · 2016-10-12T16:21:18Z

@shurcooL I have a "Closes" line in my commit message already.

gsquire · 2016-10-19T18:23:00Z

Rebased onto master again.

Closes google#437

gmlewis · 2016-10-20T21:02:46Z

Hmmm... it looks like I dropped the ball on this one... I'm sorry about that... Merging.

gmlewis · 2016-10-20T21:17:16Z

Ah, yes... thanks, @shurcooL for the reminder. This was held up on the header issue.
I haven't had a chance to test the comma-separated header yet. Hopefully today.
I'll test before I merge.

eparis · 2016-10-20T21:19:31Z

This breaks the existing 'squash' support. Our API is changing.

I don't even know if squash is still in the github API though. Or if we care (I don't).

I'll rebase my work to support SHA on top of this. Great!

gmlewis · 2016-10-20T21:21:00Z

SGTM. Yes, "squash" is still supported in the GitHub API via the MergeMethod.
Thank you, @eparis!

eparis · 2016-10-20T21:22:33Z

@gmlewis but our API is breaking in this PR is what I was trying to say, not that I care, just pointing it out in case anyone else wanted to complain that we know this will cause code to be unable to compile.

gmlewis · 2016-10-20T21:26:09Z

Ah! I didn't understand that, @eparis. Can you please copy/paste the error message(s)?

eparis · 2016-10-20T21:26:21Z

 // PullRequestOptions lets you define how a pull request will be merged.
 type PullRequestOptions struct {
-	Squash bool
+	CommitTitle string


Shouldn't these be *string? Or should we check for != "" before we assign to pullRequestMergeRequest? Otherwise setting any PullRequestOptions will result in all options be set (to explicit "")

As far as I can tell from https://developer.github.com/changes/2016-09-26-pull-request-merge-api-update/ and https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button
this field is mandatory, although below we make it omitempty.
Yes, I'm thinking that if we just test for != "" before assigning to pullRequestMergeRequest below, that this would be safe.

eparis · 2016-10-20T21:29:45Z


 // PullRequestOptions lets you define how a pull request will be merged.
 type PullRequestOptions struct {
-	Squash bool


by removing this any user of our library who is doing

ops := &PullRequestOptions { Squash: true, }

will be unable to compile. We "could" keep supporting them by automatically turning Squash into a MergeMethod = "squash" if we so chose. We don't have to. If github breaks the API I don't know if we really need to be forward compatible with our users....

Yes, GitHub is changing their API.
Please read: https://developer.github.com/changes/2016-04-01-squash-api-preview/

But we don't have to. We may decide we want to, and that's fine. But it is easy to write:

type PullRequestOptions struct { Squash bool MergeMethod string }

And then inside https://github.com/google/go-github/pull/446/files#diff-aafac035cce1f6a65b02566c5345b805R276

if options.Squash { mm := "squash" pullRequestBody.MergeMethod = &mm }

Just pointing it out. I'm actually ok dropping OUR legacy API and break compile for our users. But its also pretty easy to be a friendly library if that's the path we wish to take.

Hmmm... @willnorris and @shurcooL - what are your thoughts about breaking the current (preview) API involving "squashes" when the GitHub Preview API itself is changing? Do we match the GitHub API or do we attempt to retain a "more friendly" API toward squashes?
See: https://developer.github.com/changes/2016-04-01-squash-api-preview/ for more info.

To remain consistent with the rest of this package, I'm leaning toward matching the GitHub API as close as possible to not create any surprises for users. But if you feel we should depart from this consistency and make the API friendlier for squashing, I'm not adverse to that either.

I just caught up on this thread. I'll standby until we settle on what this API should have. Please feel free to ping me so I can make the appropriate changes.

Thanks for all of the comments!

Hmmm... @willnorris and @shurcooL - what are your thoughts about breaking the current (preview) API involving "squashes" when the GitHub Preview API itself is changing? Do we match the GitHub API or do we attempt to retain a "more friendly" API toward squashes?

We've had this discussion in the past. Let me try to find it.

I think it's this comment and its thread that I had in mind.

Basically, my POV is that we're welcome to make breaking API changes to endpoints that are in preview. We absolutely should, that's the whole point of API preview period, it lets GitHub iterate on the API and this library should follow. Using the very first iteration is going to be counter-productive for users after the API exits preview period.

@gmlewis I'm not sure if your question is about whether it's okay to break go-github APIs for preview endpoints (IMO it is), or if it's about whether we should deviate from GitHub API in what go-github provides (IMO we should try to stay close, but still be idiomatic Go, no need to compromise on usability/friendliness).

To remain consistent with the rest of this package, I'm leaning toward matching the GitHub API as close as possible to not create any surprises for users. But if you feel we should depart from this consistency and make the API friendlier for squashing, I'm not adverse to that either.

I think your question is about the latter. Can you elaborate on what 2 alternatives you're considering here?

Also, we should open an issue that this PR resolves and continue the discussion there. It's hard to see this "outdated comment" in this PR.

Creating new issue for discussion, now that GitHub no longer appears to be under DDOS attack.

gsquire · 2016-11-09T20:03:58Z

@gmlewis I hope this PR hasn't gone stale. Based on the issue you wrote discussing how we should handle the merge method, the code is already set up to do that.

Please let me know if there are any more items I should address in the meantime.

gmlewis · 2016-11-09T20:10:05Z

Thanks for the ping, @gsquire, and I apologize for the delay!
AFAICT, this PR is ready to be merged, and I'll go ahead and do that now.

gmlewis · 2016-11-09T20:14:36Z

Integrated in 575b0b3.

gsquire · 2016-11-09T20:17:21Z

@gmlewis It's no problem, thanks for your patience. 👍

Closes google#437. Closes google#446. Change-Id: I7a9bd0717310ef016b2695fe0e5cffb8e8b86ff8

gmlewis reviewed Oct 10, 2016

View reviewed changes

gsquire commented Oct 10, 2016

View reviewed changes

gmlewis reviewed Oct 10, 2016

View reviewed changes

gmlewis added the lgtm label Oct 11, 2016

gsquire force-pushed the api-updates branch from 24fbcea to 2e1a7fb Compare October 12, 2016 00:46

dmitshur changed the title ~~Issue 437~~ Add support for merge method and repository squash settings. Oct 12, 2016

dmitshur reviewed Oct 12, 2016

View reviewed changes

gsquire force-pushed the api-updates branch from 2e1a7fb to fed57d2 Compare October 19, 2016 18:22

add support for merge method and repository squash settings

7f84a6d

Closes google#437

gsquire force-pushed the api-updates branch from 714f6b3 to 7f84a6d Compare October 20, 2016 16:47

gmlewis mentioned this pull request Oct 20, 2016

Support SHA in merges #456

Merged

eparis reviewed Oct 20, 2016

View reviewed changes

gmlewis mentioned this pull request Oct 24, 2016

Discuss Pull Request Merge (squash) API #457

Closed

gmlewis closed this in 575b0b3 Nov 9, 2016

gsquire deleted the api-updates branch November 9, 2016 20:14

gmlewis mentioned this pull request Apr 10, 2017

Remove custom media type for Merge Methods API #616

Closed

bubg-dev pushed a commit to bubg-dev/go-github that referenced this pull request Jun 16, 2017

add support for merge method and repository squash settings

1ac3e4c

Closes google#437. Closes google#446. Change-Id: I7a9bd0717310ef016b2695fe0e5cffb8e8b86ff8

		@@ -251,12 +251,14 @@ type PullRequestMergeResult struct {

		// PullRequestOptions lets you define how a pull request will be merged.
		type PullRequestOptions struct {

		@@ -268,10 +270,12 @@ func (s *PullRequestsService) Merge(owner string, repo string, number int, commi
		pullRequestBody := &pullRequestMergeRequest{CommitMessage: &commitMessage}
		if options != nil {

Conversation

gsquire commented Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmlewis Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsquire left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmlewis commented Oct 10, 2016

Uh oh!

gsquire commented Oct 10, 2016

Uh oh!

gsquire commented Oct 12, 2016

Uh oh!

dmitshur commented Oct 12, 2016

Uh oh!

dmitshur left a comment

Choose a reason for hiding this comment

Uh oh!

dmitshur Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsquire commented Oct 12, 2016

Uh oh!

gsquire commented Oct 19, 2016

Uh oh!

gmlewis commented Oct 20, 2016

Uh oh!

gmlewis commented Oct 20, 2016

Uh oh!

eparis commented Oct 20, 2016

Uh oh!

gsquire commented Oct 10, 2016 •

edited

Loading

gmlewis Oct 10, 2016 •

edited

Loading

dmitshur Oct 12, 2016 •

edited

Loading

eparis Oct 20, 2016 •

edited

Loading

eparis Oct 20, 2016 •

edited

Loading