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

check PR changed files count directly from get pull request endpoint #875

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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 pull/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,4 +255,5 @@ type Body struct {
CreatedAt time.Time
Author string
LastEditedAt time.Time
ChangedFiles int
Copy link
Member

Choose a reason for hiding this comment

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

Since it looks like nothing uses this field after calling the Body() method, I think should remove it for now and only add it in if it is required at some later point. The same thing would apply to the modification to the v4PullRequestWithEditedAt struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. I have removed both fields from these structs.

}
15 changes: 12 additions & 3 deletions pull/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ func (loc Locator) IsComplete() bool {
case loc.Value.GetHead().GetRepo().GetID() == 0:
case loc.Value.GetHead().GetRepo().GetName() == "":
case loc.Value.GetHead().GetRepo().GetOwner().GetLogin() == "":
case loc.Value.GetChangedFiles() == 0:
default:
return true
}
Expand Down Expand Up @@ -108,6 +109,7 @@ func (loc Locator) toV4(ctx context.Context, client *githubv4.Client) (*v4PullRe
v4.BaseRefName = loc.Value.GetBase().GetRef()
v4.BaseRepository.DatabaseID = loc.Value.GetBase().GetRepo().GetID()
v4.IsDraft = loc.Value.GetDraft()
v4.ChangedFiles = loc.Value.GetChangedFiles()
return &v4, nil
}

Expand Down Expand Up @@ -207,6 +209,7 @@ type v4PullRequestWithEditedAt struct {
CreatedAt time.Time
LastEditedAt time.Time
Body string
ChangedFiles int
}

func (ghc *GitHubContext) Body() (*Body, error) {
Expand All @@ -230,6 +233,7 @@ func (ghc *GitHubContext) Body() (*Body, error) {
CreatedAt: graphqlResponse.CreatedAt,
Author: graphqlResponse.Author.GetV3Login(),
LastEditedAt: graphqlResponse.LastEditedAt,
ChangedFiles: graphqlResponse.ChangedFiles,
}, nil
}

Expand Down Expand Up @@ -270,6 +274,11 @@ func (ghc *GitHubContext) Branches() (base string, head string) {
}

func (ghc *GitHubContext) ChangedFiles() ([]*File, error) {
// Check if changed files exceeds the limit
if ghc.pr.ChangedFiles > MaxPullRequestFiles {
return nil, errors.Errorf("number of changed files (%d) exceeds limit (%d)", ghc.pr.ChangedFiles, MaxPullRequestFiles)
}

if ghc.files == nil {
opt := github.ListOptions{
PerPage: 100,
Expand Down Expand Up @@ -318,9 +327,7 @@ func (ghc *GitHubContext) ChangedFiles() ([]*File, error) {
})
}
}
if len(ghc.files) >= MaxPullRequestFiles {
return nil, errors.Errorf("too many files in pull request, maximum is %d", MaxPullRequestFiles)
}

return ghc.files, nil
}

Expand Down Expand Up @@ -1109,6 +1116,8 @@ type v4PullRequest struct {
BaseRepository struct {
DatabaseID int64
}

ChangedFiles int
}

type v4PageInfo struct {
Expand Down
22 changes: 22 additions & 0 deletions pull/github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,27 @@ func TestChangedFiles(t *testing.T) {
assert.Equal(t, 2, filesRule.Count, "cached files were not used")
}

func TestChangedFilesExceedsLimit(t *testing.T) {
rp := &ResponsePlayer{}
rp.AddRule(
ExactPathMatcher("/repos/testorg/testrepo/pulls/123/files"),
"testdata/responses/pull_files.yml",
)
rp.AddRule(
GraphQLNodePrefixMatcher("repository.pullRequest.changedFiles"),
"testdata/responses/pull_changed_files_exceeds_limit.yml",
)

pr := defaultTestPR()
*pr.ChangedFiles = 3001

ctx := makeContext(t, rp, pr, nil)

_, err := ctx.ChangedFiles()
assert.Equal(t, 3001, pr.GetChangedFiles())
assert.Contains(t, err.Error(), "number of changed files (3001) exceeds limit (3000)")
}

func TestChangedFilesNoFiles(t *testing.T) {
rp := &ResponsePlayer{}
filesRule := rp.AddRule(
Expand Down Expand Up @@ -723,6 +744,7 @@ func defaultTestPR() *github.PullRequest {
Name: github.String("testrepo"),
},
},
ChangedFiles: github.Int(1),
}
}

Expand Down
18 changes: 18 additions & 0 deletions pull/testdata/responses/pull_changed_files_exceeds_limit.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
- status: 200
body: |
{
"errors": [],
"data": {
"repository": {
"pullRequest": {
"body": "/no-platform",
"createdAt": "2018-01-12T00:11:50Z",
"lastEditedAt": "2018-01-12T00:11:50Z",
"author": {
"login": "agirlnamedsophia"
},
"changedFiles": 3001
}
}
}
}