Skip to content

fix(bitbucket): avoid extra plans using a local LRU #3402

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

Merged
merged 19 commits into from
Jul 7, 2023

Conversation

Almenon
Copy link
Contributor

@Almenon Almenon commented May 11, 2023

what

  • We only need to run plan if the code changes
  • The code changes if the git SHA changes
  • Therefore, we can skip the plan if the SHA is the same
  • To check if the SHA's are the same, we need to compare against the last SHA
  • We store the last SHA in a LRU cache keyed by PR url

why

tests

Unit tests. I also built the binary and ran Atlantis manually with a few test scenarios.

references

Sidenote: I ran into an error installing pegomock, I suggest upgrading it. I'm not going to upgrade it in this PR because it would change all the generated files and make a scary monster PR.

➜  atlantis git:(main) go install github.com/petergtz/pegomock/pegomock
../go/pkg/mod/github.com/petergtz/[email protected]+incompatible/modelgen/loader/loader.go:10:2: missing go.sum entry for module providing package golang.org/x/tools/go/loader (imported by github.com/petergtz/pegomock/modelgen/loader); to add:
	go get github.com/petergtz/pegomock/modelgen/[email protected]+incompatible
../go/pkg/mod/github.com/petergtz/[email protected]+incompatible/pegomock/watch/watch.go:25:2: missing go.sum entry for module providing package gopkg.in/alecthomas/kingpin.v2 (imported by github.com/petergtz/pegomock/pegomock); to add:
	go get github.com/petergtz/pegomock/[email protected]+incompatible

The latest pegomock is under a different path: github.com/petergtz/pegomock/v4/pegomock@latest

@Almenon Almenon requested a review from a team as a code owner May 11, 2023 22:22
@github-actions github-actions bot added dependencies PRs that update a dependency file go Pull requests that update Go code labels May 11, 2023
@Almenon
Copy link
Contributor Author

Almenon commented May 11, 2023

Note that people are still going to run into extra plans in these scenarios:

  • Restart of Atlantis
  • Multiple Atlantis servers behind redis, if a request is routed to a different server that doesn't have the SHA in the cache.

So the solution isn't perfect, but it's better than nothing.

@nitrocode nitrocode changed the title Fix Bitbucket extra plans fix(bitbucket): extra plans May 11, 2023
@nitrocode nitrocode changed the title fix(bitbucket): extra plans fix(bitbucket): avoid extra plans using a local LRU May 11, 2023
Co-authored-by: nitrocode <[email protected]>
@GenPage GenPage added this to the v0.25.0 milestone May 13, 2023
@github-actions github-actions bot added the docs Documentation label May 20, 2023
@Almenon
Copy link
Contributor Author

Almenon commented Jun 9, 2023

Based on @GenPage 's comment not sure whether the PR can be merged currently, seeking clarification on that.

If we store events in boltDB I'll have to throw out my existing code and rework the PR. I'd need to expire old events so DB doesn't fill up indefinitely, so each key would a value date,sha and every PR I'd have to scan all the keys, filter down to the PR prefix, then delete any key older than 2 months.

It would involve additional work and complexity compared to the current PR, for a situation that would be rendered irrelevant when bitbucket changes their webhooks again, so I'd prefer to keep the PR as-is.

GenPage
GenPage previously approved these changes Jun 20, 2023
Copy link
Member

@GenPage GenPage left a comment

Choose a reason for hiding this comment

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

@Almenon I will not block the PR. It's apparent we don't have any other options regarding this without refactoring the PR model and involving the DB. Please resolve the branch conflicts with go.mod and we will merge.

@Almenon
Copy link
Contributor Author

Almenon commented Jun 21, 2023

@GenPage The conflicts are resolved. Glad to hear it can be merged :D. This will be my first open-source Go PR so I'm excited! It looks like a maintainer needs to approve some of the CI jobs for them to run by the way.

@Almenon Almenon requested review from GenPage and nitrocode June 21, 2023 00:09
@GenPage GenPage enabled auto-merge (squash) July 7, 2023 22:34
Copy link
Member

@GenPage GenPage left a comment

Choose a reason for hiding this comment

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

@Almenon congrats on your first contribution

:shipit:

@GenPage GenPage merged commit 78632be into runatlantis:main Jul 7, 2023
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
terakoya76 pushed a commit to terakoya76/atlantis that referenced this pull request Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies PRs that update a dependency file docs Documentation go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Atlantis runs plan when bitbucket pr review request or description changed
4 participants