From 78632beb20ebaf3f91d1da406a8401e76b27074a Mon Sep 17 00:00:00 2001 From: Almenon Date: Fri, 7 Jul 2023 15:35:22 -0700 Subject: [PATCH] fix(bitbucket): avoid extra plans using a local LRU (#3402) --- .gitignore | 3 ++ go.mod | 1 + go.sum | 3 ++ runatlantis.io/docs/autoplanning.md | 6 +++ .../controllers/events/events_controller.go | 3 +- server/events/event_parser.go | 14 ++++++- server/events/event_parser_test.go | 40 ++++++++++++++++++- server/events/mocks/mock_event_parsing.go | 4 +- 8 files changed, 68 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index 2d9dad12f1..a3040a1ee5 100644 --- a/.gitignore +++ b/.gitignore @@ -24,3 +24,6 @@ dist/ tmp-CHANGELOG.md .envrc + +# IDE files +*.code-workspace diff --git a/go.mod b/go.mod index 0cb57eb6e1..0cb4622f78 100644 --- a/go.mod +++ b/go.mod @@ -20,6 +20,7 @@ require ( github.com/hashicorp/go-getter/v2 v2.2.1 github.com/hashicorp/go-multierror v1.1.1 github.com/hashicorp/go-version v1.6.0 + github.com/hashicorp/golang-lru/v2 v2.0.2 github.com/hashicorp/terraform-config-inspect v0.0.0-20230614215431-f32df32a01cd github.com/kr/pretty v0.3.1 github.com/mcdafydd/go-azuredevops v0.12.1 diff --git a/go.sum b/go.sum index 94da5c5ea8..431376fa2d 100644 --- a/go.sum +++ b/go.sum @@ -253,6 +253,9 @@ github.com/hashicorp/go-version v1.6.0 h1:feTTfFNnjP967rlCxM/I9g701jU+RN74YKx2mO github.com/hashicorp/go-version v1.6.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA= github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= +github.com/hashicorp/golang-lru v0.5.4 h1:YDjusn29QI/Das2iO9M0BHnIbxPeyuCHsjMW+lJfyTc= +github.com/hashicorp/golang-lru/v2 v2.0.2 h1:Dwmkdr5Nc/oBiXgJS3CDHNhJtIHkuZ3DZF5twqnfBdU= +github.com/hashicorp/golang-lru/v2 v2.0.2/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM= github.com/hashicorp/hcl v1.0.0 h1:0Anlzjpi4vEasTeNFn2mLJgTSwt0+6sfsiTG8qcWGx4= github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ= github.com/hashicorp/hcl/v2 v2.17.0 h1:z1XvSUyXd1HP10U4lrLg5e0JMVz6CPaJvAgxM0KNZVY= diff --git a/runatlantis.io/docs/autoplanning.md b/runatlantis.io/docs/autoplanning.md index 3461013980..2183219703 100644 --- a/runatlantis.io/docs/autoplanning.md +++ b/runatlantis.io/docs/autoplanning.md @@ -32,6 +32,12 @@ Given the directory structure: * If `project1/modules/module1/main.tf` were modified, we would look one level above `project1/modules` into `project1/`, see that there was a `main.tf` file and so run plan in `project1/` +## Bitbucket-Specific Notes +Bitbucket does not have a webhook that triggers only upon a new PR or commit. To fix this we cache the last commit to see if it has changed. If the cache is emptied, Atlantis will think your commit is new and you may see extra plans. +This scenario can happen if: +* Atlantis restarts +* You are running multiple Atlantis instances behind a load balancer + ## Customizing If you would like to customize how Atlantis determines which directory to run in or disable it all together you need to create an `atlantis.yaml` file. diff --git a/server/controllers/events/events_controller.go b/server/controllers/events/events_controller.go index 0742e09065..c4f1b53474 100644 --- a/server/controllers/events/events_controller.go +++ b/server/controllers/events/events_controller.go @@ -366,7 +366,8 @@ func (e *VCSEventsController) handleBitbucketCloudPullRequestEvent(w http.Respon e.respond(w, logging.Error, http.StatusBadRequest, "Error parsing pull data: %s %s=%s", err, bitbucketCloudRequestIDHeader, reqID) return } - pullEventType := e.Parser.GetBitbucketCloudPullEventType(eventType) + e.Logger.Debug("SHA is %q", pull.HeadCommit) + pullEventType := e.Parser.GetBitbucketCloudPullEventType(eventType, pull.HeadCommit, pull.URL) e.Logger.Info("identified event as type %q", pullEventType.String()) resp := e.handlePullRequestEvent(e.Logger, baseRepo, headRepo, pull, user, pullEventType) diff --git a/server/events/event_parser.go b/server/events/event_parser.go index 220dd3b697..1ab99938ad 100644 --- a/server/events/event_parser.go +++ b/server/events/event_parser.go @@ -22,6 +22,7 @@ import ( "github.com/go-playground/validator/v10" "github.com/google/go-github/v53/github" + lru "github.com/hashicorp/golang-lru/v2" "github.com/mcdafydd/go-azuredevops/azuredevops" "github.com/pkg/errors" "github.com/runatlantis/atlantis/server/events/command" @@ -34,6 +35,8 @@ import ( const gitlabPullOpened = "opened" const usagesCols = 90 +var lastBitbucketSha, _ = lru.New[string, string](300) + // PullCommand is a command to run on a pull request. type PullCommand interface { // CommandName is the name of the command we're running. @@ -267,7 +270,7 @@ type EventParsing interface { // GetBitbucketCloudPullEventType returns the type of the pull request // event given the Bitbucket Cloud header. - GetBitbucketCloudPullEventType(eventTypeHeader string) models.PullRequestEventType + GetBitbucketCloudPullEventType(eventTypeHeader string, sha string, pr string) models.PullRequestEventType // ParseBitbucketServerPullEvent parses a pull request event from Bitbucket // Server. @@ -343,11 +346,18 @@ func (e *EventParser) ParseAPIPlanRequest(vcsHostType models.VCSHostType, repoFu // GetBitbucketCloudPullEventType returns the type of the pull request // event given the Bitbucket Cloud header. -func (e *EventParser) GetBitbucketCloudPullEventType(eventTypeHeader string) models.PullRequestEventType { +func (e *EventParser) GetBitbucketCloudPullEventType(eventTypeHeader string, sha string, pr string) models.PullRequestEventType { switch eventTypeHeader { case bitbucketcloud.PullCreatedHeader: + lastBitbucketSha.Add(pr, sha) return models.OpenedPullEvent case bitbucketcloud.PullUpdatedHeader: + lastSha, _ := lastBitbucketSha.Get(pr) + if sha == lastSha { + // No change, ignore + return models.OtherPullEvent + } + lastBitbucketSha.Add(pr, sha) return models.UpdatedPullEvent case bitbucketcloud.PullFulfilledHeader, bitbucketcloud.PullRejectedHeader: return models.ClosedPullEvent diff --git a/server/events/event_parser_test.go b/server/events/event_parser_test.go index efc5b37df7..fc7fc38917 100644 --- a/server/events/event_parser_test.go +++ b/server/events/event_parser_test.go @@ -998,6 +998,42 @@ func TestParseBitbucketCloudPullEvent_States(t *testing.T) { } } +func TestBitBucketNonCodeChangesAreIgnored(t *testing.T) { + // lets say a user opens a PR + act := parser.GetBitbucketCloudPullEventType("pullrequest:created", "fakeSha", "https://github.com/fakeorg/fakerepo/pull/1") + Equals(t, models.OpenedPullEvent, act) + // Another update with same SHA should be ignored + act = parser.GetBitbucketCloudPullEventType("pullrequest:updated", "fakeSha", "https://github.com/fakeorg/fakerepo/pull/1") + Equals(t, models.OtherPullEvent, act) + // Only if SHA changes do we act + act = parser.GetBitbucketCloudPullEventType("pullrequest:updated", "fakeSha2", "https://github.com/fakeorg/fakerepo/pull/1") + Equals(t, models.UpdatedPullEvent, act) + + // If sha changes in seperate PR, + act = parser.GetBitbucketCloudPullEventType("pullrequest:updated", "otherPRSha", "https://github.com/fakeorg/fakerepo/pull/2") + Equals(t, models.UpdatedPullEvent, act) + // We will still ignore same shas in first PR + act = parser.GetBitbucketCloudPullEventType("pullrequest:updated", "fakeSha2", "https://github.com/fakeorg/fakerepo/pull/1") + Equals(t, models.OtherPullEvent, act) +} + +func TestBitbucketShaCacheExpires(t *testing.T) { + // lets say a user opens a PR + act := parser.GetBitbucketCloudPullEventType("pullrequest:created", "fakeSha", "https://github.com/fakeorg/fakerepo/pull/1") + Equals(t, models.OpenedPullEvent, act) + // Another update with same SHA should be ignored + act = parser.GetBitbucketCloudPullEventType("pullrequest:updated", "fakeSha", "https://github.com/fakeorg/fakerepo/pull/1") + Equals(t, models.OtherPullEvent, act) + // But after 300 times, the cache should expire + // this is so we don't have ever increasing memory usage + for i := 0; i < 302; i++ { + parser.GetBitbucketCloudPullEventType("pullrequest:updated", "fakeSha", fmt.Sprintf("https://github.com/fakeorg/fakerepo/pull/%d", i)) + } + // and now SHA will seen as a change again + act = parser.GetBitbucketCloudPullEventType("pullrequest:updated", "fakeSha", "https://github.com/fakeorg/fakerepo/pull/1") + Equals(t, models.UpdatedPullEvent, act) +} + func TestGetBitbucketCloudEventType(t *testing.T) { cases := []struct { header string @@ -1026,7 +1062,9 @@ func TestGetBitbucketCloudEventType(t *testing.T) { } for _, c := range cases { t.Run(c.header, func(t *testing.T) { - act := parser.GetBitbucketCloudPullEventType(c.header) + // we pass in the header as the SHA so the SHA changes each time + // the code will ignore duplicate SHAS to avoid extra TF plans + act := parser.GetBitbucketCloudPullEventType(c.header, c.header, "https://github.com/fakeorg/fakerepo/pull/1") Equals(t, c.exp, act) }) } diff --git a/server/events/mocks/mock_event_parsing.go b/server/events/mocks/mock_event_parsing.go index c2433a1f37..ef45393891 100644 --- a/server/events/mocks/mock_event_parsing.go +++ b/server/events/mocks/mock_event_parsing.go @@ -28,11 +28,11 @@ func NewMockEventParsing(options ...pegomock.Option) *MockEventParsing { func (mock *MockEventParsing) SetFailHandler(fh pegomock.FailHandler) { mock.fail = fh } func (mock *MockEventParsing) FailHandler() pegomock.FailHandler { return mock.fail } -func (mock *MockEventParsing) GetBitbucketCloudPullEventType(eventTypeHeader string) models.PullRequestEventType { +func (mock *MockEventParsing) GetBitbucketCloudPullEventType(eventTypeHeader string, sha string, pr string) models.PullRequestEventType { if mock == nil { panic("mock must not be nil. Use myMock := NewMockEventParsing().") } - params := []pegomock.Param{eventTypeHeader} + params := []pegomock.Param{eventTypeHeader, sha, pr} result := pegomock.GetGenericMockFrom(mock).Invoke("GetBitbucketCloudPullEventType", params, []reflect.Type{reflect.TypeOf((*models.PullRequestEventType)(nil)).Elem()}) var ret0 models.PullRequestEventType if len(result) != 0 {