Skip to content

Add support for merge method and repository squash settings. #446

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

Closed
gsquire wants to merge 1 commit into from
+25 −7
Closed

Add support for merge method and repository squash settings. #446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions github/github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ const (
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/
mediaTypeSquashPreview = "application/vnd.github.polaris-preview+json"

// https://developer.github.com/changes/2016-04-04-git-signing-api-preview/
Expand Down
11 changes: 8 additions & 3 deletions github/pulls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,12 +253,16 @@ type PullRequestMergeResult struct {

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

@gmlewis gmlewis Oct 10, 2016

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@gsquire gsquire Oct 10, 2016

Choose a reason for hiding this comment

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

Yes, I certainly missed that in the blog post.

Squash bool
CommitTitle string
Copy link
Copy Markdown
Contributor

@eparis eparis Oct 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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 "")

Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Oct 24, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@eparis eparis Oct 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Oct 20, 2016

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@eparis eparis Oct 20, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Oct 20, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@gsquire gsquire Oct 20, 2016

Choose a reason for hiding this comment

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

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!

Copy link
Copy Markdown
Member

@dmitshur dmitshur Oct 20, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Oct 24, 2016

Choose a reason for hiding this comment

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

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


// The merge method to use. Possible values include: "merge", "squash", and "rebase" with the default being merge.
MergeMethod string
Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Oct 10, 2016

Choose a reason for hiding this comment

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

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

}

type pullRequestMergeRequest struct {
Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Oct 10, 2016

Choose a reason for hiding this comment

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

You will need to add CommitTitle here too.

CommitMessage *string `json:"commit_message"`
Squash *bool `json:"squash,omitempty"`
CommitTitle *string `json:"commit_title,omitempty"`
MergeMethod *string `json:"merge_method,omitempty"`
}

// Merge a pull request (Merge Button™).
Expand All @@ -269,7 +273,8 @@ func (s *PullRequestsService) Merge(owner string, repo string, number int, commi

pullRequestBody := &pullRequestMergeRequest{CommitMessage: &commitMessage}
if options != nil {
Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis Oct 10, 2016

Choose a reason for hiding this comment

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

And update this section too.

pullRequestBody.Squash = &options.Squash
pullRequestBody.CommitTitle = &options.CommitTitle
pullRequestBody.MergeMethod = &options.MergeMethod
}
req, err := s.client.NewRequest("PUT", u, pullRequestBody)

Expand Down
2 changes: 1 addition & 1 deletion github/pulls_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ func TestPullRequestsService_Merge(t *testing.T) {
}`)
})

options := &PullRequestOptions{Squash: true}
options := &PullRequestOptions{MergeMethod: "rebase"}
merge, _, err := client.PullRequests.Merge("o", "r", 1, "merging pull request", options)
if err != nil {
t.Errorf("PullRequests.Merge returned error: %v", err)
Expand Down
14 changes: 12 additions & 2 deletions github/repos.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@

package github

import "fmt"
import (
"fmt"
"strings"
)

// RepositoriesService handles communication with the repository related
// methods of the GitHub API.
Expand Down Expand Up @@ -46,6 +49,9 @@ type Repository struct {
Source *Repository `json:"source,omitempty"`
Organization *Organization `json:"organization,omitempty"`
Permissions *map[string]bool `json:"permissions,omitempty"`
AllowRebaseMerge *bool `json:"allow_rebase_merge,omitempty"`
AllowSquashMerge *bool `json:"allow_squash_merge,omitempty"`
AllowMergeCommit *bool `json:"allow_merge_commit,omitempty"`

// Only provided when using RepositoriesService.Get while in preview
License *License `json:"license,omitempty"`
Expand Down Expand Up @@ -286,7 +292,8 @@ func (s *RepositoriesService) Get(owner, repo string) (*Repository, *Response, e

// 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)
acceptHeaders := []string{mediaTypeLicensesPreview, mediaTypeSquashPreview}
req.Header.Set("Accept", strings.Join(acceptHeaders, ", "))

repository := new(Repository)
resp, err := s.client.Do(req, repository)
Expand Down Expand Up @@ -330,6 +337,9 @@ func (s *RepositoriesService) Edit(owner, repo string, repository *Repository) (
return nil, nil, err
}

// TODO: Remove this preview header after API is fully vetted.
req.Header.Add("Accept", mediaTypeSquashPreview)

r := new(Repository)
resp, err := s.client.Do(req, r)
if err != nil {
Expand Down
4 changes: 3 additions & 1 deletion github/repos_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"fmt"
"net/http"
"reflect"
"strings"
"testing"
)

Expand Down Expand Up @@ -225,9 +226,10 @@ func TestRepositoriesService_Get(t *testing.T) {
setup()
defer teardown()

acceptHeader := []string{mediaTypeLicensesPreview, mediaTypeSquashPreview}
mux.HandleFunc("/repos/o/r", func(w http.ResponseWriter, r *http.Request) {
testMethod(t, r, "GET")
testHeader(t, r, "Accept", mediaTypeLicensesPreview)
testHeader(t, r, "Accept", strings.Join(acceptHeader, ", "))
fmt.Fprint(w, `{"id":1,"name":"n","description":"d","owner":{"login":"l"},"license":{"key":"mit"}}`)
})

Expand Down