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

feat(automerge): implement GitHub --auto-merge-method flag for apply command #4895

Merged
merged 6 commits into from
Nov 2, 2024
Merged

feat(automerge): implement GitHub --auto-merge-method flag for apply command #4895

merged 6 commits into from
Nov 2, 2024

Conversation

a1k0u
Copy link
Contributor

@a1k0u a1k0u commented Sep 4, 2024 •
edited
Loading

what

Implemented flag which allows to specify merge method for the VCS (GitHub only for now) in automerge process.

atlantis apply --auto-merge-method merge
atlantis apply --auto-merge-method rebase
atlantis apply --auto-merge-method squash

why

In our company, Atlantis is used not only by DevOps/SRE/Ops engineers, but also by many developers from other teams. They have a different style of git commit messages, a different way of dividing code changes into commits. Sometimes we need to ask them to squash commits/rewrite the commit message.

This causes problems - after first review, the developers force changes (squash commits/rewrite the commit message), and then start the second atlantis plan, which must be reviewed a second time. Squash fixes this because the result commit message is a PR header that can be changed without any changes to the git history.

In the old logic, all methods except squash can be disabled, and this will fix the previous case, but maintenance engineers often work with a large task where a single pull request contains several independent commits that can be revert after merging into master.

Thus, these two cases conflict with each other, because we need to use different merge strategies in different situations.
This can be fixed if any flag is able to control the merge method for automerge function.

tests

I have tested my changes by extend TestGithubClient_MergePullCorrectMethod. Added tests in which the --auto-merge-method flag has the correct value, where correct value is not allowed and where value is not correct.

references

(opened issue) Automerge support for "require linear history" #1176
(opened issue) Feature Request: Allow Auto Merge with Squash or Rebase option #2047

@a1k0u a1k0u requested review from a team as code owners September 4, 2024 09:26
@a1k0u a1k0u requested review from GenPage, lukemassa and X-Guardian and removed request for a team September 4, 2024 09:26
@github-actions github-actions bot added docs Documentation go Pull requests that update Go code provider/github labels Sep 4, 2024
@chenrui333 chenrui333 added feature New functionality/enhancement and removed docs Documentation labels Sep 5, 2024
chenrui333
chenrui333 previously approved these changes Sep 12, 2024
View reviewed changes
@chenrui333
Copy link
Member

@a1k0u Thanks for implementing this feature. Looks like there is some conflict before merging the PR.

@github-actions github-actions bot added the docs Documentation label Sep 13, 2024
@a1k0u
Copy link
Contributor Author

a1k0u commented Sep 13, 2024 •
edited
Loading

@chenrui333
Thank you for review! I've resolved the conflict

@a1k0u
Copy link
Contributor Author

a1k0u commented Sep 25, 2024

@chenrui333, hi! Can you review this PR again, please?

@a1k0u a1k0u force-pushed the specify-merge-method branch 2 times, most recently from 2d2aebe to 138d163 Compare October 7, 2024 14:40
@a1k0u
Copy link
Contributor Author

a1k0u commented Oct 7, 2024

@chenrui333 @jamengual @X-Guardian, hi! Can you review this PR again, please?

@a1k0u a1k0u force-pushed the specify-merge-method branch 3 times, most recently from 022e7eb to 2fded7d Compare October 14, 2024 08:09
@a1k0u
Copy link
Contributor Author

a1k0u commented Nov 1, 2024 •
edited
Loading

@chenrui333 @jamengual @X-Guardian @GenPage @lukemassa, hi! Any updates?

@jamengual
Copy link
Contributor

@a1k0u, could you please sign the commit? sorry we have not gotten to your pr, but we have been more busy than normal

@a1k0u a1k0u force-pushed the specify-merge-method branch 2 times, most recently from 683404c to 63dfbea Compare November 1, 2024 19:46
@a1k0u
Copy link
Contributor Author

a1k0u commented Nov 1, 2024

@jamengual, thank you! I've signed off the commit

X-Guardian
X-Guardian requested changes Nov 2, 2024
View reviewed changes
Copy link
Contributor

@X-Guardian X-Guardian left a comment

Choose a reason for hiding this comment

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

I think this option should be called automerge-method rather than merge-method to be clear that it is the method to be used for an auto-merge and not a normal merge.

runatlantis.io/docs/automerging.md Outdated Show resolved Hide resolved
runatlantis.io/docs/automerging.md Outdated Show resolved Hide resolved
@X-Guardian X-Guardian added the waiting-on-response Waiting for a response from the user label Nov 2, 2024
@a1k0u a1k0u force-pushed the specify-merge-method branch 2 times, most recently from 4b02fb3 to cf87803 Compare November 2, 2024 14:13
@a1k0u a1k0u changed the title feat(automerge): implement --merge-method flag for apply command feat(automerge): implement --auto-merge-method flag for apply command Nov 2, 2024
@X-Guardian X-Guardian changed the title feat(automerge): implement --auto-merge-method flag for apply command feat(automerge): implement GitHub --auto-merge-method flag for apply command Nov 2, 2024
X-Guardian
X-Guardian requested changes Nov 2, 2024
View reviewed changes
Copy link
Contributor

@X-Guardian X-Guardian left a comment

Choose a reason for hiding this comment

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

Thanks @a1k0u. I've tested this on GitHub and it is working as expected, and on GitLab it is showing the correct error message.

Just some minor refinement to the error messages and we are good to go.

server/events/vcs/github_client.go Outdated Show resolved Hide resolved
server/events/vcs/github_client.go Outdated Show resolved Hide resolved
server/events/vcs/github_client_test.go Outdated Show resolved Hide resolved
server/events/vcs/github_client_test.go Outdated Show resolved Hide resolved
server/events/vcs/github_client_test.go Outdated Show resolved Hide resolved
server/events/vcs/github_client_test.go Outdated Show resolved Hide resolved
server/events/vcs/github_client_test.go Outdated Show resolved Hide resolved
server/events/comment_parser.go Outdated Show resolved Hide resolved
server/events/comment_parser.go Outdated Show resolved Hide resolved
server/events/comment_parser.go Outdated Show resolved Hide resolved
a1k0u and others added 5 commits November 2, 2024 22:30
@a1k0u
feat(automerge): implement --merge-method flag for apply command
a1b4141 
Signed-off-by: a1k0u <alko7enko@gmail.com>
@a1k0u
chore(automerge): rename flag to --auto-merge-method
ce661d3 
Signed-off-by: a1k0u <alko7enko@gmail.com>
@renovate @X-Guardian @a1k0u
fix(deps): update module github.com/google/go-github/v63 to v65 in go…
87bcd94 
….mod (main) (runatlantis#4968)

Signed-off-by: X-Guardian <simonheather99@gmail.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: X-Guardian <simonheather99@gmail.com>
Signed-off-by: a1k0u <alko7enko@gmail.com>
@a1k0u @X-Guardian
chore(automerge): fix typo, improve error messages
1a3ff66 
Co-authored-by: Simon Heather <32168619+X-Guardian@users.noreply.github.com>
Signed-off-by: Alexey Kosenko <alko7enko@gmail.com>
Signed-off-by: a1k0u <alko7enko@gmail.com>
@a1k0u
fix(automerge): update ApplyUsage in comment_parset_test
63d8e7e 
Signed-off-by: a1k0u <alko7enko@gmail.com>
@github-actions github-actions bot added the dependencies PRs that update a dependency file label Nov 2, 2024
@a1k0u
Copy link
Contributor Author

a1k0u commented Nov 2, 2024 •
edited
Loading

@X-Guardian, thx! It looks great :)
I've merged your suggestions and updated comment_parser_test

@X-Guardian X-Guardian enabled auto-merge (squash) November 2, 2024 19:50
@X-Guardian X-Guardian merged commit 13fa635 into runatlantis:main Nov 2, 2024
34 checks passed
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 2, 2024
@X-Guardian
Copy link
Contributor

X-Guardian commented Nov 2, 2024 •
edited
Loading

Thanks for your work on this @a1k0u. You can test it by using one of the following container images: dev-alpine-13fa635 or dev-debian-13fa635

ajax-ryzhyi-r pushed a commit to ajax-ryzhyi-r/atlantis that referenced this pull request Nov 3, 2024
@a1k0u @ajax-ryzhyi-r
feat(automerge): implement GitHub --auto-merge-method flag for apply …
e69c8d9 
…command (runatlantis#4895)

Signed-off-by: a1k0u <alko7enko@gmail.com>
Signed-off-by: X-Guardian <simonheather99@gmail.com>
ajax-ryzhyi-r pushed a commit to ajax-ryzhyi-r/atlantis that referenced this pull request Nov 3, 2024
@a1k0u @ajax-ryzhyi-r
feat(automerge): implement GitHub --auto-merge-method flag for apply …
e8c00be 
…command (runatlantis#4895)

Signed-off-by: a1k0u <alko7enko@gmail.com>
Signed-off-by: X-Guardian <simonheather99@gmail.com>
a1k0u added a commit to tufitko/atlantis that referenced this pull request Nov 6, 2024
@a1k0u
feat(automerge): implement GitHub --auto-merge-method flag for apply …
e95c2d3 
…command (runatlantis#4895)

Signed-off-by: a1k0u <alko7enko@gmail.com>
Signed-off-by: X-Guardian <simonheather99@gmail.com>
@BrewTestBot BrewTestBot mentioned this pull request Nov 22, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@X-Guardian X-Guardian X-Guardian approved these changes

@GenPage GenPage Awaiting requested review from GenPage GenPage is a code owner automatically assigned from runatlantis/maintainers

@lukemassa lukemassa Awaiting requested review from lukemassa lukemassa is a code owner automatically assigned from runatlantis/core-contributors

@chenrui333 chenrui333 Awaiting requested review from chenrui333

Assignees
No one assigned
Labels
dependencies PRs that update a dependency file docs Documentation feature New functionality/enhancement go Pull requests that update Go code lgtm This PR has been approved by a maintainer provider/github waiting-on-response Waiting for a response from the user
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@a1k0u @chenrui333 @jamengual @X-Guardian