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

Allow "Mergeable" apply requirement to work #454

Closed
wants to merge 1 commit into from
Closed

Allow "Mergeable" apply requirement to work #454

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
3 changes: 3 additions & 0 deletions server/events/command_context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,7 @@ type CommandContext struct {
// User is the user that triggered this command.
User models.User
Log *logging.SimpleLogger
// Mergeable tells us whether Pull is mergeable before changing the status of the Pull
// PullMergeable is true if Pull is able to be merged.
PullMergeable bool
}
14 changes: 13 additions & 1 deletion server/events/command_runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ package events

import (
"fmt"

"github.com/google/go-github/github"
"github.com/lkysow/go-gitlab"
"github.com/pkg/errors"
Expand Down Expand Up @@ -167,7 +168,18 @@ func (c *DefaultCommandRunner) RunCommentCommand(baseRepo models.Repo, maybeHead
if !c.validateCtxAndComment(ctx) {
return
}
if err = c.CommitStatusUpdater.Update(baseRepo, pull, models.PendingCommitStatus, cmd.CommandName()); err != nil {

if cmd.CommandName() == ApplyCommand {
ctx.PullMergeable, err = c.VCSClient.PullIsMergeable(baseRepo, pull)
if err != nil {
// On error we continue the request with mergeable assumed false.
// We want to continue because not all apply's will need this status,
// only if they rely on the mergeability requirement.
ctx.PullMergeable = false
ctx.Log.Warn("unable to get mergeable status: %s. Continuing with mergeable assumed false", err)
}
}
if err = c.CommitStatusUpdater.Update(ctx.BaseRepo, ctx.Pull, models.PendingCommitStatus, cmd.CommandName()); err != nil {
ctx.Log.Warn("unable to update commit status: %s", err)
}

Expand Down
6 changes: 4 additions & 2 deletions server/events/models/models.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,10 @@ type ProjectCommandContext struct {
// If the pull request branch is from the same repository then HeadRepo will
// be the same as BaseRepo.
// See https://help.github.com/articles/about-pull-request-merges/.
HeadRepo Repo
Log *logging.SimpleLogger
HeadRepo Repo
Log *logging.SimpleLogger
// PullMergeable is true if the pull request for this project is able to be merged.
PullMergeable bool
Pull PullRequest
ProjectConfig *valid.Project
// RePlanCmd is the command that users should run to re-plan this project.
Expand Down
1 change: 1 addition & 0 deletions server/events/project_command_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,7 @@ func (p *DefaultProjectCommandBuilder) buildProjectCommandCtx(ctx *CommandContex
GlobalConfig: globalCfg,
RePlanCmd: p.CommentBuilder.BuildPlanComment(repoRelDir, workspace, projectName, commentFlags),
ApplyCmd: p.CommentBuilder.BuildApplyComment(repoRelDir, workspace, projectName),
PullMergeable: ctx.PullMergeable,
}, nil
}

Expand Down
11 changes: 3 additions & 8 deletions server/events/project_command_runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ type DefaultProjectCommandRunner struct {
ApplyStepRunner StepRunner
RunStepRunner StepRunner
PullApprovedChecker runtime.PullApprovedChecker
PullMergeableChecker runtime.PullMergeableChecker
WorkingDir WorkingDir
Webhooks WebhooksSender
WorkingDirLocker WorkingDirLocker
Expand Down Expand Up @@ -144,7 +143,7 @@ func (p *DefaultProjectCommandRunner) doPlan(ctx models.ProjectCommandContext) (
return nil, "", cloneErr
}
projAbsPath := filepath.Join(repoDir, ctx.RepoRelDir)
if _, err := os.Stat(projAbsPath); os.IsNotExist(err) {
if _, err = os.Stat(projAbsPath); os.IsNotExist(err) {
return nil, "", DirNotExistErr{RepoRelDir: ctx.RepoRelDir}
}

Expand Down Expand Up @@ -209,7 +208,7 @@ func (p *DefaultProjectCommandRunner) doApply(ctx models.ProjectCommandContext)
return "", "", err
}
absPath := filepath.Join(repoDir, ctx.RepoRelDir)
if _, err := os.Stat(absPath); os.IsNotExist(err) {
if _, err = os.Stat(absPath); os.IsNotExist(err) {
return "", "", DirNotExistErr{RepoRelDir: ctx.RepoRelDir}
}

Expand Down Expand Up @@ -238,11 +237,7 @@ func (p *DefaultProjectCommandRunner) doApply(ctx models.ProjectCommandContext)
return "", "Pull request must be approved before running apply.", nil
}
case raw.MergeableApplyRequirement:
mergeable, err := p.PullMergeableChecker.PullIsMergeable(ctx.BaseRepo, ctx.Pull) // nolint: vetshadow
pratikmallya marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return "", "", errors.Wrap(err, "checking if pull request is mergeable")
}
if !mergeable {
if !ctx.PullMergeable {
return "", "Pull request must be mergeable before running apply.", nil
}
}
Expand Down
124 changes: 73 additions & 51 deletions server/events/project_command_runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,17 +134,16 @@ func TestDefaultProjectCommandRunner_Plan(t *testing.T) {
mockLocker := mocks.NewMockProjectLocker()

runner := events.DefaultProjectCommandRunner{
Locker: mockLocker,
LockURLGenerator: mockURLGenerator{},
InitStepRunner: mockInit,
PlanStepRunner: mockPlan,
ApplyStepRunner: mockApply,
RunStepRunner: mockRun,
PullApprovedChecker: nil,
PullMergeableChecker: nil,
WorkingDir: mockWorkingDir,
Webhooks: nil,
WorkingDirLocker: events.NewDefaultWorkingDirLocker(),
Locker: mockLocker,
LockURLGenerator: mockURLGenerator{},
InitStepRunner: mockInit,
PlanStepRunner: mockPlan,
ApplyStepRunner: mockApply,
RunStepRunner: mockRun,
PullApprovedChecker: nil,
WorkingDir: mockWorkingDir,
Webhooks: nil,
WorkingDirLocker: events.NewDefaultWorkingDirLocker(),
}

repoDir, cleanup := TempDir(t)
Expand Down Expand Up @@ -236,37 +235,37 @@ func TestDefaultProjectCommandRunner_ApplyNotApproved(t *testing.T) {
func TestDefaultProjectCommandRunner_ApplyNotMergeable(t *testing.T) {
RegisterMockTestingT(t)
mockWorkingDir := mocks.NewMockWorkingDir()
mockMergeable := mocks2.NewMockPullMergeableChecker()
runner := &events.DefaultProjectCommandRunner{
WorkingDir: mockWorkingDir,
PullMergeableChecker: mockMergeable,
WorkingDirLocker: events.NewDefaultWorkingDirLocker(),
RequireMergeableOverride: true,
}
ctx := models.ProjectCommandContext{}
tmp, cleanup := TempDir(t)
defer cleanup()
When(mockWorkingDir.GetWorkingDir(ctx.BaseRepo, ctx.Pull, ctx.Workspace)).ThenReturn(tmp, nil)
When(mockMergeable.PullIsMergeable(ctx.BaseRepo, ctx.Pull)).ThenReturn(false, nil)

res := runner.Apply(ctx)
Equals(t, "Pull request must be mergeable before running apply.", res.Failure)
}

func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
cases := []struct {
description string
projCfg *valid.Project
globalCfg *valid.Config
expSteps []string
expOut string
description string
projCfg *valid.Project
globalCfg *valid.Config
expSteps []string
expOut string
expFailure string
pullMergeable bool
}{
{
description: "use defaults",
projCfg: nil,
globalCfg: nil,
expSteps: []string{"apply"},
expOut: "apply",
description: "use defaults",
projCfg: nil,
globalCfg: nil,
expSteps: []string{"apply"},
expOut: "apply",
pullMergeable: true,
},
{
description: "no workflow, use defaults",
Expand All @@ -281,8 +280,9 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
},
},
},
expSteps: []string{"apply"},
expOut: "apply",
expSteps: []string{"apply"},
expOut: "apply",
pullMergeable: true,
},
{
description: "no workflow, approval required, use defaults",
Expand All @@ -299,8 +299,9 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
},
},
},
expSteps: []string{"approve", "apply"},
expOut: "apply",
expSteps: []string{"approve", "apply"},
expOut: "apply",
pullMergeable: true,
},
{
description: "no workflow, mergeable required, use defaults",
Expand All @@ -317,8 +318,29 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
},
},
},
expSteps: []string{"mergeable", "apply"},
expOut: "apply",
expSteps: []string{"apply"},
expOut: "apply",
pullMergeable: true,
},
{
description: "no workflow, mergeable required, use defaults, commit unmergeable",
projCfg: &valid.Project{
Dir: ".",
ApplyRequirements: []string{"mergeable"},
},
globalCfg: &valid.Config{
Version: 2,
Projects: []valid.Project{
{
Dir: ".",
ApplyRequirements: []string{"mergeable"},
},
},
},
expSteps: []string{""},
expOut: "",
expFailure: "Pull request must be mergeable before running apply.",
pullMergeable: false,
},
{
description: "no workflow, mergeable and approved required, use defaults",
Expand All @@ -335,8 +357,9 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
},
},
},
expSteps: []string{"mergeable", "approved", "apply"},
expOut: "apply",
expSteps: []string{"approved", "apply"},
expOut: "apply",
pullMergeable: true,
},
{
description: "workflow without apply stage set",
Expand All @@ -359,8 +382,9 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
},
},
},
expSteps: []string{"apply"},
expOut: "apply",
expSteps: []string{"apply"},
expOut: "apply",
pullMergeable: true,
},
{
description: "workflow with custom apply stage",
Expand Down Expand Up @@ -396,8 +420,9 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
},
},
},
expSteps: []string{"run", "apply", "plan", "init"},
expOut: "run\napply\nplan\ninit",
expSteps: []string{"run", "apply", "plan", "init"},
expOut: "run\napply\nplan\ninit",
pullMergeable: true,
},
}

Expand All @@ -409,23 +434,21 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
mockApply := mocks.NewMockStepRunner()
mockRun := mocks.NewMockStepRunner()
mockApproved := mocks2.NewMockPullApprovedChecker()
mockMergeable := mocks2.NewMockPullMergeableChecker()
mockWorkingDir := mocks.NewMockWorkingDir()
mockLocker := mocks.NewMockProjectLocker()
mockSender := mocks.NewMockWebhooksSender()

runner := events.DefaultProjectCommandRunner{
Locker: mockLocker,
LockURLGenerator: mockURLGenerator{},
InitStepRunner: mockInit,
PlanStepRunner: mockPlan,
ApplyStepRunner: mockApply,
RunStepRunner: mockRun,
PullApprovedChecker: mockApproved,
PullMergeableChecker: mockMergeable,
WorkingDir: mockWorkingDir,
Webhooks: mockSender,
WorkingDirLocker: events.NewDefaultWorkingDirLocker(),
Locker: mockLocker,
LockURLGenerator: mockURLGenerator{},
InitStepRunner: mockInit,
PlanStepRunner: mockPlan,
ApplyStepRunner: mockApply,
RunStepRunner: mockRun,
PullApprovedChecker: mockApproved,
WorkingDir: mockWorkingDir,
Webhooks: mockSender,
WorkingDirLocker: events.NewDefaultWorkingDirLocker(),
}
repoDir, cleanup := TempDir(t)
defer cleanup()
Expand All @@ -441,23 +464,22 @@ func TestDefaultProjectCommandRunner_Apply(t *testing.T) {
Workspace: "default",
GlobalConfig: c.globalCfg,
RepoRelDir: ".",
PullMergeable: c.pullMergeable,
}
When(mockInit.Run(ctx, nil, repoDir)).ThenReturn("init", nil)
When(mockPlan.Run(ctx, nil, repoDir)).ThenReturn("plan", nil)
When(mockApply.Run(ctx, nil, repoDir)).ThenReturn("apply", nil)
When(mockRun.Run(ctx, nil, repoDir)).ThenReturn("run", nil)
When(mockApproved.PullIsApproved(ctx.BaseRepo, ctx.Pull)).ThenReturn(true, nil)
When(mockMergeable.PullIsMergeable(ctx.BaseRepo, ctx.Pull)).ThenReturn(true, nil)

res := runner.Apply(ctx)
Equals(t, c.expOut, res.ApplySuccess)
Equals(t, c.expFailure, res.Failure)

for _, step := range c.expSteps {
switch step {
case "approved":
mockApproved.VerifyWasCalledOnce().PullIsApproved(ctx.BaseRepo, ctx.Pull)
case "mergeable":
mockMergeable.VerifyWasCalledOnce().PullIsMergeable(ctx.BaseRepo, ctx.Pull)
case "init":
mockInit.VerifyWasCalledOnce().Run(ctx, nil, repoDir)
case "plan":
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test case that tests your new code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would that work? Sorry, I'm not sure how this mocking framework works, but it seems like you're testing if a function is being called; whereas in the case of testing Mergeability, its just checking the value of ctx.PullMergeable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lkysow Added a test case to test the failure path. not sure if that's what you were asking for; lemme know

Copy link
Member

Choose a reason for hiding this comment

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

That's perfect

Expand Down
Loading