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.

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

Jump to bottom

Move checks for pulls before merge into own function (#19271) #19277

Merged
merged 10 commits into from
Mar 31, 2022
Merged

Move checks for pulls before merge into own function (#19271) #19277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5462912
move checks for branchprotection into own package
6543 Mar 30, 2022
63a5e53
resolve todo now ...
6543 Mar 30, 2022
6db4511
move & rename
6543 Mar 30, 2022
91f07d4
unexport if posible
6543 Mar 30, 2022
132ddff
wording
6543 Mar 31, 2022
a0abc81
improve SetDefaults & nits
6543 Mar 31, 2022
919cc15
fix test
6543 Mar 31, 2022
0322944
merge files
6543 Mar 31, 2022
562f199
use package "errors"
6543 Mar 31, 2022
84a8a4c
merge files
6543 Mar 31, 2022
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
27 changes: 11 additions & 16 deletions models/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,22 +222,19 @@ func (pr *PullRequest) loadProtectedBranch(ctx context.Context) (err error) {
}

// GetDefaultMergeMessage returns default message used when merging pull request
func (pr *PullRequest) GetDefaultMergeMessage() string {
func (pr *PullRequest) GetDefaultMergeMessage() (string, error) {
if pr.HeadRepo == nil {
var err error
pr.HeadRepo, err = repo_model.GetRepositoryByID(pr.HeadRepoID)
if err != nil {
log.Error("GetRepositoryById[%d]: %v", pr.HeadRepoID, err)
return ""
return "", fmt.Errorf("GetRepositoryById[%d]: %v", pr.HeadRepoID, err)
}
}
if err := pr.LoadIssue(); err != nil {
log.Error("Cannot load issue %d for PR id %d: Error: %v", pr.IssueID, pr.ID, err)
return ""
return "", fmt.Errorf("Cannot load issue %d for PR id %d: Error: %v", pr.IssueID, pr.ID, err)
}
if err := pr.LoadBaseRepo(); err != nil {
log.Error("LoadBaseRepo: %v", err)
return ""
return "", fmt.Errorf("LoadBaseRepo: %v", err)
}

issueReference := "#"
Expand All @@ -246,10 +243,10 @@ func (pr *PullRequest) GetDefaultMergeMessage() string {
}

if pr.BaseRepoID == pr.HeadRepoID {
return fmt.Sprintf("Merge pull request '%s' (%s%d) from %s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadBranch, pr.BaseBranch)
return fmt.Sprintf("Merge pull request '%s' (%s%d) from %s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadBranch, pr.BaseBranch), nil
}

return fmt.Sprintf("Merge pull request '%s' (%s%d) from %s:%s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseBranch)
return fmt.Sprintf("Merge pull request '%s' (%s%d) from %s:%s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseBranch), nil
}

// ReviewCount represents a count of Reviews
Expand Down Expand Up @@ -335,19 +332,17 @@ func (pr *PullRequest) getReviewedByLines(writer io.Writer) error {
}

// GetDefaultSquashMessage returns default message used when squash and merging pull request
func (pr *PullRequest) GetDefaultSquashMessage() string {
func (pr *PullRequest) GetDefaultSquashMessage() (string, error) {
if err := pr.LoadIssue(); err != nil {
log.Error("LoadIssue: %v", err)
return ""
return "", fmt.Errorf("LoadIssue: %v", err)
}
if err := pr.LoadBaseRepo(); err != nil {
log.Error("LoadBaseRepo: %v", err)
return ""
return "", fmt.Errorf("LoadBaseRepo: %v", err)
}
if pr.BaseRepo.UnitEnabled(unit.TypeExternalTracker) {
return fmt.Sprintf("%s (!%d)", pr.Issue.Title, pr.Issue.Index)
return fmt.Sprintf("%s (!%d)", pr.Issue.Title, pr.Issue.Index), nil
}
return fmt.Sprintf("%s (#%d)", pr.Issue.Title, pr.Issue.Index)
return fmt.Sprintf("%s (#%d)", pr.Issue.Title, pr.Issue.Index), nil
}

// GetGitRefName returns git ref for hidden pull request branch
Expand Down
16 changes: 12 additions & 4 deletions models/pull_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,11 +261,15 @@ func TestPullRequest_GetDefaultMergeMessage_InternalTracker(t *testing.T) {
assert.NoError(t, unittest.PrepareTestDatabase())
pr := unittest.AssertExistsAndLoadBean(t, &PullRequest{ID: 2}).(*PullRequest)

assert.Equal(t, "Merge pull request 'issue3' (#3) from branch2 into master", pr.GetDefaultMergeMessage())
msg, err := pr.GetDefaultMergeMessage()
assert.NoError(t, err)
assert.Equal(t, "Merge pull request 'issue3' (#3) from branch2 into master", msg)

pr.BaseRepoID = 1
pr.HeadRepoID = 2
assert.Equal(t, "Merge pull request 'issue3' (#3) from user2/repo1:branch2 into master", pr.GetDefaultMergeMessage())
msg, err = pr.GetDefaultMergeMessage()
assert.NoError(t, err)
assert.Equal(t, "Merge pull request 'issue3' (#3) from user2/repo1:branch2 into master", msg)
}

func TestPullRequest_GetDefaultMergeMessage_ExternalTracker(t *testing.T) {
Expand All @@ -283,9 +287,13 @@ func TestPullRequest_GetDefaultMergeMessage_ExternalTracker(t *testing.T) {

pr := unittest.AssertExistsAndLoadBean(t, &PullRequest{ID: 2, BaseRepo: baseRepo}).(*PullRequest)

assert.Equal(t, "Merge pull request 'issue3' (!3) from branch2 into master", pr.GetDefaultMergeMessage())
msg, err := pr.GetDefaultMergeMessage()
assert.NoError(t, err)
assert.Equal(t, "Merge pull request 'issue3' (!3) from branch2 into master", msg)

pr.BaseRepoID = 1
pr.HeadRepoID = 2
assert.Equal(t, "Merge pull request 'issue3' (!3) from user2/repo1:branch2 into master", pr.GetDefaultMergeMessage())
msg, err = pr.GetDefaultMergeMessage()
assert.NoError(t, err)
assert.Equal(t, "Merge pull request 'issue3' (!3) from user2/repo1:branch2 into master", msg)
}
104 changes: 28 additions & 76 deletions routers/api/v1/repo/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ func ListPullRequests(ctx *context.APIContext) {
Labels: ctx.FormStrings("labels"),
MilestoneID: ctx.FormInt64("milestone"),
})

if err != nil {
ctx.Error(http.StatusInternalServerError, "PullRequests", err)
return
Expand Down Expand Up @@ -724,13 +723,12 @@ func MergePullRequest(ctx *context.APIContext) {
return
}

if err = pr.LoadHeadRepo(); err != nil {
if err := pr.LoadHeadRepo(); err != nil {
ctx.Error(http.StatusInternalServerError, "LoadHeadRepo", err)
return
}

err = pr.LoadIssue()
if err != nil {
if err := pr.LoadIssue(); err != nil {
ctx.Error(http.StatusInternalServerError, "LoadIssue", err)
return
}
Expand All @@ -744,29 +742,33 @@ func MergePullRequest(ctx *context.APIContext) {
}
}

if pr.Issue.IsClosed {
ctx.NotFound()
return
}
manuallMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged
force := form.ForceMerge != nil && *form.ForceMerge

allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, ctx.Repo.Permission, ctx.User)
if err != nil {
ctx.Error(http.StatusInternalServerError, "IsUSerAllowedToMerge", err)
return
}
if !allowedMerge {
ctx.Error(http.StatusMethodNotAllowed, "Merge", "User not allowed to merge PR")
return
}

if pr.HasMerged {
ctx.Error(http.StatusMethodNotAllowed, "PR already merged", "")
if err := pull_service.CheckPullMergable(ctx, ctx.User, &ctx.Repo.Permission, pr, manuallMerge, force); err != nil {
if errors.Is(err, pull_service.ErrIsClosed) {
ctx.NotFound()
} else if errors.Is(err, pull_service.ErrUserNotAllowedToMerge) {
ctx.Error(http.StatusMethodNotAllowed, "Merge", "User not allowed to merge PR")
} else if errors.Is(err, pull_service.ErrHasMerged) {
ctx.Error(http.StatusMethodNotAllowed, "PR already merged", "")
} else if errors.Is(err, pull_service.ErrIsWorkInProgress) {
ctx.Error(http.StatusMethodNotAllowed, "PR is a work in progress", "Work in progress PRs cannot be merged")
} else if errors.Is(err, pull_service.ErrNotMergableState) {
ctx.Error(http.StatusMethodNotAllowed, "PR not in mergeable state", "Please try again later")
} else if models.IsErrNotAllowedToMerge(err) {
ctx.Error(http.StatusMethodNotAllowed, "PR is not ready to be merged", err)
} else if asymkey_service.IsErrWontSign(err) {
ctx.Error(http.StatusMethodNotAllowed, fmt.Sprintf("Protected branch %s requires signed commits but this merge would not be signed", pr.BaseBranch), err)
} else {
ctx.InternalServerError(err)
}
return
}

// handle manually-merged mark
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged {
if err = pull_service.MergedManually(pr, ctx.User, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
if manuallMerge {
if err := pull_service.MergedManually(pr, ctx.User, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
if models.IsErrInvalidMergeStyle(err) {
ctx.Error(http.StatusMethodNotAllowed, "Invalid merge style", fmt.Errorf("%s is not allowed an allowed merge style for this repository", repo_model.MergeStyle(form.Do)))
return
Expand All @@ -782,63 +784,13 @@ func MergePullRequest(ctx *context.APIContext) {
return
}

if !pr.CanAutoMerge() {
ctx.Error(http.StatusMethodNotAllowed, "PR not in mergeable state", "Please try again later")
return
}

if pr.IsWorkInProgress() {
ctx.Error(http.StatusMethodNotAllowed, "PR is a work in progress", "Work in progress PRs cannot be merged")
// set defaults to propagate needed fields
if err := form.SetDefaults(pr); err != nil {
ctx.ServerError("SetDefaults", fmt.Errorf("SetDefaults: %v", err))
return
}

if err := pull_service.CheckPRReadyToMerge(pr, false); err != nil {
if !models.IsErrNotAllowedToMerge(err) {
ctx.Error(http.StatusInternalServerError, "CheckPRReadyToMerge", err)
return
}
if form.ForceMerge != nil && *form.ForceMerge {
if isRepoAdmin, err := models.IsUserRepoAdmin(pr.BaseRepo, ctx.User); err != nil {
ctx.Error(http.StatusInternalServerError, "IsUserRepoAdmin", err)
return
} else if !isRepoAdmin {
ctx.Error(http.StatusMethodNotAllowed, "Merge", "Only repository admin can merge if not all checks are ok (force merge)")
}
} else {
ctx.Error(http.StatusMethodNotAllowed, "PR is not ready to be merged", err)
return
}
}

if _, err := pull_service.IsSignedIfRequired(pr, ctx.User); err != nil {
if !asymkey_service.IsErrWontSign(err) {
ctx.Error(http.StatusInternalServerError, "IsSignedIfRequired", err)
return
}
ctx.Error(http.StatusMethodNotAllowed, fmt.Sprintf("Protected branch %s requires signed commits but this merge would not be signed", pr.BaseBranch), err)
return
}

if len(form.Do) == 0 {
form.Do = string(repo_model.MergeStyleMerge)
}

message := strings.TrimSpace(form.MergeTitleField)
if len(message) == 0 {
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleMerge {
message = pr.GetDefaultMergeMessage()
}
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleSquash {
message = pr.GetDefaultSquashMessage()
}
}

form.MergeMessageField = strings.TrimSpace(form.MergeMessageField)
if len(form.MergeMessageField) > 0 {
message += "\n\n" + form.MergeMessageField
}

if err := pull_service.Merge(pr, ctx.User, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message); err != nil {
if err := pull_service.Merge(pr, ctx.User, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, form.MergeTitleField); err != nil {
if models.IsErrInvalidMergeStyle(err) {
ctx.Error(http.StatusMethodNotAllowed, "Invalid merge style", fmt.Errorf("%s is not allowed an allowed merge style for this repository", repo_model.MergeStyle(form.Do)))
return
Expand Down
Loading