Skip to content

Commit

Permalink
fix: Atlantis Does Not Consider the Plan Directory When Hiding Previo…
Browse files Browse the repository at this point in the history
…us Plan Comments (#4012)

* Fix Hide Previous Plan Comments

* Update GitLab client tests

* Update GitLab client test

* Update github client test

* Add nolint: errcheck to test

* format github_client.go
  • Loading branch information
X-Guardian authored Dec 29, 2023
1 parent 8f8c4d0 commit 3180267
Show file tree
Hide file tree
Showing 15 changed files with 261 additions and 161 deletions.
10 changes: 7 additions & 3 deletions server/controllers/events/events_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -586,9 +586,13 @@ func (e *VCSEventsController) handleCommentEvent(logger logging.SimpleLogging, b
body: "Commenting back on pull request",
}
}

logger.Info("Running comment command '%v' on repo '%v', pull request: %v for user '%v'.",
parseResult.Command.Name, baseRepo.FullName, pullNum, user.Username)
if parseResult.Command.RepoRelDir != "" {
logger.Info("Running comment command '%v' on dir '%v' on repo '%v', pull request: %v for user '%v'.",
parseResult.Command.Name, parseResult.Command.RepoRelDir, baseRepo.FullName, pullNum, user.Username)
} else {
logger.Info("Running comment command '%v' on repo '%v', pull request: %v for user '%v'.",
parseResult.Command.Name, baseRepo.FullName, pullNum, user.Username)
}
if !e.TestingMode {
// Respond with success and then actually execute the command asynchronously.
// We use a goroutine so that this function returns and the connection is
Expand Down
18 changes: 18 additions & 0 deletions server/events/event_parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ var lastBitbucketSha, _ = lru.New[string, string](300)

// PullCommand is a command to run on a pull request.
type PullCommand interface {
// Dir is the path relative to the repo root to run the command in.
// Will never end in "/". If empty then the comment specified no directory.
Dir() string
// CommandName is the name of the command we're running.
CommandName() command.Name
// SubCommandName is the subcommand name of the command we're running.
Expand All @@ -63,6 +66,11 @@ func (c PolicyCheckCommand) SubCommandName() string {
return ""
}

// Dir is empty
func (c PolicyCheckCommand) Dir() string {
return ""
}

// IsVerbose is false for policy_check commands.
func (c PolicyCheckCommand) IsVerbose() bool {
return false
Expand All @@ -87,6 +95,11 @@ func (c AutoplanCommand) SubCommandName() string {
return ""
}

// Dir is empty
func (c AutoplanCommand) Dir() string {
return ""
}

// IsVerbose is false for autoplan commands.
func (c AutoplanCommand) IsVerbose() bool {
return false
Expand Down Expand Up @@ -133,6 +146,11 @@ func (c CommentCommand) IsForSpecificProject() bool {
return c.RepoRelDir != "" || c.Workspace != "" || c.ProjectName != ""
}

// Dir returns the dir of this command.
func (c CommentCommand) Dir() string {
return c.RepoRelDir
}

// CommandName returns the name of this command.
func (c CommentCommand) CommandName() command.Name {
return c.Name
Expand Down
3 changes: 2 additions & 1 deletion server/events/pull_updater.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ func (c *PullUpdater) updatePull(ctx *command.Context, cmd PullCommand, res comm
// clutter in a pull/merge request. This will not delete the comment, since the
// comment trail may be useful in auditing or backtracing problems.
if c.HidePrevPlanComments {
if err := c.VCSClient.HidePrevCommandComments(ctx.Pull.BaseRepo, ctx.Pull.Num, cmd.CommandName().TitleString()); err != nil {
ctx.Log.Debug("Hiding previous plan comments for command: '%v', directory: '%v'", cmd.CommandName().TitleString(), cmd.Dir())
if err := c.VCSClient.HidePrevCommandComments(ctx.Pull.BaseRepo, ctx.Pull.Num, cmd.CommandName().TitleString(), cmd.Dir()); err != nil {
ctx.Log.Err("unable to hide old comments: %s", err)
}
}
Expand Down
2 changes: 1 addition & 1 deletion server/events/vcs/azuredevops_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ func (g *AzureDevopsClient) ReactToComment(repo models.Repo, pullNum int, commen
return nil
}

func (g *AzureDevopsClient) HidePrevCommandComments(repo models.Repo, pullNum int, command string) error { //nolint: revive
func (g *AzureDevopsClient) HidePrevCommandComments(repo models.Repo, pullNum int, command string, dir string) error { //nolint: revive
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion server/events/vcs/bitbucketcloud/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func (b *Client) ReactToComment(_ models.Repo, _ int, _ int64, _ string) error {
return nil
}

func (b *Client) HidePrevCommandComments(_ models.Repo, _ int, _ string) error {
func (b *Client) HidePrevCommandComments(_ models.Repo, _ int, _ string, _ string) error {
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion server/events/vcs/bitbucketserver/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ func (b *Client) ReactToComment(_ models.Repo, _ int, _ int64, _ string) error {
return nil
}

func (b *Client) HidePrevCommandComments(_ models.Repo, _ int, _ string) error {
func (b *Client) HidePrevCommandComments(_ models.Repo, _ int, _ string, _ string) error {
return nil
}

Expand Down
4 changes: 2 additions & 2 deletions server/events/vcs/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
"github.com/runatlantis/atlantis/server/events/models"
)

//go:generate pegomock generate --package mocks -o mocks/mock_client.go Client
//go:generate pegomock generate --package mocks -o mocks/mock_client.go github.com/runatlantis/atlantis/server/events/vcs Client

// Client is used to make API calls to a VCS host like GitHub or GitLab.
type Client interface {
Expand All @@ -27,7 +27,7 @@ type Client interface {
CreateComment(repo models.Repo, pullNum int, comment string, command string) error

ReactToComment(repo models.Repo, pullNum int, commentID int64, reaction string) error
HidePrevCommandComments(repo models.Repo, pullNum int, command string) error
HidePrevCommandComments(repo models.Repo, pullNum int, command string, dir string) error
PullIsApproved(repo models.Repo, pull models.PullRequest) (models.ApprovalStatus, error)
PullIsMergeable(repo models.Repo, pull models.PullRequest, vcsstatusname string) (bool, error)
// UpdateStatus updates the commit status to state for pull. src is the
Expand Down
9 changes: 8 additions & 1 deletion server/events/vcs/github_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ func (g *GithubClient) ReactToComment(repo models.Repo, _ int, commentID int64,
return err
}

func (g *GithubClient) HidePrevCommandComments(repo models.Repo, pullNum int, command string) error {
func (g *GithubClient) HidePrevCommandComments(repo models.Repo, pullNum int, command string, dir string) error {
var allComments []*github.IssueComment
nextPage := 0
for {
Expand Down Expand Up @@ -252,6 +252,12 @@ func (g *GithubClient) HidePrevCommandComments(repo models.Repo, pullNum int, co
if !strings.Contains(firstLine, strings.ToLower(command)) {
continue
}

// If dir was specified, skip processing comments that don't contain the dir in the first line
if dir != "" && !strings.Contains(firstLine, strings.ToLower(dir)) {
continue
}

var m struct {
MinimizeComment struct {
MinimizedComment struct {
Expand All @@ -265,6 +271,7 @@ func (g *GithubClient) HidePrevCommandComments(repo models.Repo, pullNum int, co
Classifier: githubv4.ReportedContentClassifiersOutdated,
SubjectID: comment.GetNodeID(),
}
g.logger.Debug("Hiding comment %s", comment.GetNodeID())
if err := g.v4Client.Mutate(g.ctx, &m, input, nil); err != nil {
return errors.Wrapf(err, "minimize comment %s", comment.GetNodeID())
}
Expand Down
171 changes: 103 additions & 68 deletions server/events/vcs/github_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ func TestGithubClient_PaginatesComments(t *testing.T) {
},
123,
command.Plan.TitleString(),
"",
)
Ok(t, err)
Equals(t, 2, len(gotMinimizeCalls))
Expand All @@ -240,86 +241,120 @@ func TestGithubClient_PaginatesComments(t *testing.T) {
}

func TestGithubClient_HideOldComments(t *testing.T) {
// Only comment 6 should be minimized, because it's by the same Atlantis bot user
// and it has "plan" in the first line of the comment body.
issueResp := `[
atlantisUser := "AtlantisUser"
pullRequestNum := 123
issueResp := strings.ReplaceAll(`[
{"node_id": "1", "body": "asd\nplan\nasd", "user": {"login": "someone-else"}},
{"node_id": "2", "body": "asd plan\nasd", "user": {"login": "someone-else"}},
{"node_id": "3", "body": "asdasdasd\nasdasdasd", "user": {"login": "someone-else"}},
{"node_id": "4", "body": "asdasdasd\nasdasdasd", "user": {"login": "user"}},
{"node_id": "5", "body": "asd\nplan\nasd", "user": {"login": "user"}},
{"node_id": "6", "body": "asd plan\nasd", "user": {"login": "user"}},
{"node_id": "7", "body": "asdasdasd", "user": {"login": "user"}},
{"node_id": "8", "body": "asd plan\nasd", "user": {"login": "user"}},
{"node_id": "9", "body": "Continued Plan from previous comment\nasd", "user": {"login": "user"}}
]`
{"node_id": "4", "body": "asdasdasd\nasdasdasd", "user": {"login": "AtlantisUser"}},
{"node_id": "5", "body": "asd\nplan\nasd", "user": {"login": "AtlantisUser"}},
{"node_id": "6", "body": "Ran Plan for 2 projects:", "user": {"login": "AtlantisUser"}},
{"node_id": "7", "body": "Ran Apply for 2 projects:", "user": {"login": "AtlantisUser"}},
{"node_id": "8", "body": "Ran Plan for dir: 'stack1' workspace: 'default'", "user": {"login": "AtlantisUser"}},
{"node_id": "9", "body": "Ran Plan for dir: 'stack2' workspace: 'default'", "user": {"login": "AtlantisUser"}},
{"node_id": "10", "body": "Continued Plan from previous comment\nasd", "user": {"login": "AtlantisUser"}}
]`, "'", "`")
minimizeResp := "{}"
type graphQLCall struct {
Variables struct {
Input githubv4.MinimizeCommentInput `json:"input"`
} `json:"variables"`
}
gotMinimizeCalls := make([]graphQLCall, 0, 1)
testServer := httptest.NewTLSServer(
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.Method + " " + r.RequestURI {
// This gets the pull request's comments.
case "GET /api/v3/repos/owner/repo/issues/123/comments?direction=asc&sort=created":
w.Write([]byte(issueResp)) // nolint: errcheck
return
case "POST /api/graphql":
defer r.Body.Close() // nolint: errcheck
body, err := io.ReadAll(r.Body)
if err != nil {
t.Errorf("read body error: %v", err)
http.Error(w, "server error", http.StatusInternalServerError)
return
}
call := graphQLCall{}
err = json.Unmarshal(body, &call)
if err != nil {
t.Errorf("parse body error: %v", err)
http.Error(w, "server error", http.StatusInternalServerError)
return
}
gotMinimizeCalls = append(gotMinimizeCalls, call)
w.Write([]byte(minimizeResp)) // nolint: errcheck
return
default:
t.Errorf("got unexpected request at %q", r.RequestURI)
http.Error(w, "not found", http.StatusNotFound)
return
}
}),
)

testServerURL, err := url.Parse(testServer.URL)
Ok(t, err)
cases := []struct {
dir string
processedComments int
processedCommentIds []string
}{
{
// With no dir specified, comments 6, 8, 9 and 10 should be minimized.
"",
4,
[]string{"6", "8", "9", "10"},
},
{
// With a dir of "stack1", comment 8 should be minimized.
"stack1",
1,
[]string{"8"},
},
{
// With a dir of "stack2", comment 9 should be minimized.
"stack2",
1,
[]string{"9"},
},
}

client, err := vcs.NewGithubClient(testServerURL.Host, &vcs.GithubUserCredentials{"user", "pass"}, vcs.GithubConfig{}, logging.NewNoopLogger(t))
Ok(t, err)
defer disableSSLVerification()()
for _, c := range cases {
t.Run(c.dir, func(t *testing.T) {
gotMinimizeCalls := make([]graphQLCall, 0, 1)
testServer := httptest.NewTLSServer(
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.Method + " " + r.RequestURI {
// This gets the pull request's comments.
case fmt.Sprintf("GET /api/v3/repos/owner/repo/issues/%v/comments?direction=asc&sort=created", pullRequestNum):
w.Write([]byte(issueResp)) // nolint: errcheck
return
case "POST /api/graphql":
defer r.Body.Close() // nolint: errcheck
body, err := io.ReadAll(r.Body)
if err != nil {
t.Errorf("read body error: %v", err)
http.Error(w, "server error", http.StatusInternalServerError)
return
}
call := graphQLCall{}
err = json.Unmarshal(body, &call)
if err != nil {
t.Errorf("parse body error: %v", err)
http.Error(w, "server error", http.StatusInternalServerError)
return
}
gotMinimizeCalls = append(gotMinimizeCalls, call)
w.Write([]byte(minimizeResp)) // nolint: errcheck
return
default:
t.Errorf("got unexpected request at %q", r.RequestURI)
http.Error(w, "not found", http.StatusNotFound)
return
}
}),
)

err = client.HidePrevCommandComments(
models.Repo{
FullName: "owner/repo",
Owner: "owner",
Name: "repo",
CloneURL: "",
SanitizedCloneURL: "",
VCSHost: models.VCSHost{
Hostname: "github.com",
Type: models.Github,
},
},
123,
command.Plan.TitleString(),
)
Ok(t, err)
Equals(t, 3, len(gotMinimizeCalls))
Equals(t, "6", gotMinimizeCalls[0].Variables.Input.SubjectID)
Equals(t, "9", gotMinimizeCalls[2].Variables.Input.SubjectID)
Equals(t, githubv4.ReportedContentClassifiersOutdated, gotMinimizeCalls[0].Variables.Input.Classifier)
testServerURL, err := url.Parse(testServer.URL)
Ok(t, err)

client, err := vcs.NewGithubClient(testServerURL.Host, &vcs.GithubUserCredentials{atlantisUser, "pass"}, vcs.GithubConfig{},
logging.NewNoopLogger(t))
Ok(t, err)
defer disableSSLVerification()()

err = client.HidePrevCommandComments(
models.Repo{
FullName: "owner/repo",
Owner: "owner",
Name: "repo",
CloneURL: "",
SanitizedCloneURL: "",
VCSHost: models.VCSHost{
Hostname: "github.com",
Type: models.Github,
},
},
pullRequestNum,
command.Plan.TitleString(),
c.dir,
)
Ok(t, err)
Equals(t, c.processedComments, len(gotMinimizeCalls))
for i := 0; i < c.processedComments; i++ {
Equals(t, c.processedCommentIds[i], gotMinimizeCalls[i].Variables.Input.SubjectID)
Equals(t, githubv4.ReportedContentClassifiersOutdated, gotMinimizeCalls[i].Variables.Input.Classifier)
}
})
}
}

func TestGithubClient_UpdateStatus(t *testing.T) {
Expand Down
7 changes: 6 additions & 1 deletion server/events/vcs/gitlab_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ func (g *GitlabClient) ReactToComment(repo models.Repo, pullNum int, commentID i
return err
}

func (g *GitlabClient) HidePrevCommandComments(repo models.Repo, pullNum int, command string) error {
func (g *GitlabClient) HidePrevCommandComments(repo models.Repo, pullNum int, command string, dir string) error {
var allComments []*gitlab.Note

nextPage := 0
Expand Down Expand Up @@ -250,6 +250,11 @@ func (g *GitlabClient) HidePrevCommandComments(repo models.Repo, pullNum int, co
continue
}

// If dir was specified, skip processing comments that don't contain the dir in the first line
if dir != "" && !strings.Contains(firstLine, strings.ToLower(dir)) {
continue
}

g.logger.Debug("Updating merge request note: Repo: '%s', MR: '%d', comment ID: '%d'", repo.FullName, pullNum, comment.ID)
supersededComment := summaryHeader + lineFeed + comment.Body + lineFeed + summaryFooter + lineFeed

Expand Down
Loading

0 comments on commit 3180267

Please sign in to comment.