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.

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

fix: Return error on InstrumentedClient.MergePull #3839

Conversation

inkel
Copy link
Contributor

@inkel inkel commented Oct 9, 2023

When the underlying GitHub Client returns an error it gets swallowed in this wrapper method. Not only that but also the emitted metrics are wrong, as it counts one error AND success at the same time.

We've found this @grafana when using Atlantis in a massive repository with many changes per minute, and sometimes Atlantis leave a comment saying it's automatically merging but then nothing happens. But checking the logs, we've found the following error message:

Unable to merge pull, error: merging pull request: PUT     https://api.github.com/repos/grafana/redacted/pulls/XXX/merge: 405 Base branch was modified. Review and try the merge again. []

And because the error is swallowed and InstrumentedClient.MergePull returns nil, then Automerger fails to leave a comment saying merging failed.

When the underlying GitHub Client returns an error it gets swallowed
in this wrapper method. Not only that but also the emitted metrics are
wrong, as it counts one error AND success at the same time.

We've found this @grafana when using Atlantis in a massive repository
with many changes per minute, and sometimes Atlantis leave a comment
saying it's automatically merging but then nothing happens. But
checking the logs, we've found the following error message:

    Unable to merge pull, error: merging pull request: PUT
    https://api.github.com/repos/grafana/redacted/pulls/666/merge: 405
    Base branch was modified. Review and try the merge again. []

And because the error is swallowed and `InstrumentedClient.MergePull`
returns `nil`, then `Automerger` fails to leave a comment saying
merging failed.
@inkel inkel requested a review from a team as a code owner October 9, 2023 19:11
@inkel inkel changed the title Return error on InstrumentedClient.MergePull Return error on InstrumentedClient.MergePull Oct 9, 2023
@github-actions github-actions bot added the go Pull requests that update Go code label Oct 9, 2023
@inkel inkel changed the title Return error on InstrumentedClient.MergePull bug: Return error on InstrumentedClient.MergePull Oct 9, 2023
@inkel
Copy link
Contributor Author

inkel commented Oct 9, 2023

I've fixed the title but I'm on my phone and can't re-run the check.

@jamengual jamengual changed the title bug: Return error on InstrumentedClient.MergePull fix: Return error on InstrumentedClient.MergePull Oct 9, 2023
@jamengual
Copy link
Contributor

Thanks @inkel for the contribution

@jamengual jamengual merged commit df04456 into runatlantis:main Oct 9, 2023
@inkel inkel deleted the inkel/instrumented-gh-client-return-mergepull-err branch October 10, 2023 12:26
@inkel
Copy link
Contributor Author

inkel commented Oct 10, 2023

Thank you @jamengual for the quick response!

@GenPage
Copy link
Member

GenPage commented Nov 7, 2023

/cherry-pick release-0.26

gcp-cherry-pick-bot bot pushed a commit that referenced this pull request Nov 7, 2023
When the underlying GitHub Client returns an error it gets swallowed
in this wrapper method. Not only that but also the emitted metrics are
wrong, as it counts one error AND success at the same time.

We've found this @grafana when using Atlantis in a massive repository
with many changes per minute, and sometimes Atlantis leave a comment
saying it's automatically merging but then nothing happens. But
checking the logs, we've found the following error message:

    Unable to merge pull, error: merging pull request: PUT
    https://api.github.com/repos/grafana/redacted/pulls/666/merge: 405
    Base branch was modified. Review and try the merge again. []

And because the error is swallowed and `InstrumentedClient.MergePull`
returns `nil`, then `Automerger` fails to leave a comment saying
merging failed.
GenPage pushed a commit that referenced this pull request Nov 7, 2023
When the underlying GitHub Client returns an error it gets swallowed
in this wrapper method. Not only that but also the emitted metrics are
wrong, as it counts one error AND success at the same time.

We've found this @grafana when using Atlantis in a massive repository
with many changes per minute, and sometimes Atlantis leave a comment
saying it's automatically merging but then nothing happens. But
checking the logs, we've found the following error message:

    Unable to merge pull, error: merging pull request: PUT
    https://api.github.com/repos/grafana/redacted/pulls/666/merge: 405
    Base branch was modified. Review and try the merge again. []

And because the error is swallowed and `InstrumentedClient.MergePull`
returns `nil`, then `Automerger` fails to leave a comment saying
merging failed.

Co-authored-by: Leandro López <[email protected]>
@nitrocode
Copy link
Member

This is one of the best features of atlantis 0.27.0. FYI.

Thank you.

@inkel
Copy link
Contributor Author

inkel commented Jan 22, 2024

Aaaw, thanks for saying this!

ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
When the underlying GitHub Client returns an error it gets swallowed
in this wrapper method. Not only that but also the emitted metrics are
wrong, as it counts one error AND success at the same time.

We've found this @grafana when using Atlantis in a massive repository
with many changes per minute, and sometimes Atlantis leave a comment
saying it's automatically merging but then nothing happens. But
checking the logs, we've found the following error message:

    Unable to merge pull, error: merging pull request: PUT
    https://api.github.com/repos/grafana/redacted/pulls/666/merge: 405
    Base branch was modified. Review and try the merge again. []

And because the error is swallowed and `InstrumentedClient.MergePull`
returns `nil`, then `Automerger` fails to leave a comment saying
merging failed.
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
When the underlying GitHub Client returns an error it gets swallowed
in this wrapper method. Not only that but also the emitted metrics are
wrong, as it counts one error AND success at the same time.

We've found this @grafana when using Atlantis in a massive repository
with many changes per minute, and sometimes Atlantis leave a comment
saying it's automatically merging but then nothing happens. But
checking the logs, we've found the following error message:

    Unable to merge pull, error: merging pull request: PUT
    https://api.github.com/repos/grafana/redacted/pulls/666/merge: 405
    Base branch was modified. Review and try the merge again. []

And because the error is swallowed and `InstrumentedClient.MergePull`
returns `nil`, then `Automerger` fails to leave a comment saying
merging failed.
terakoya76 pushed a commit to terakoya76/atlantis that referenced this pull request Dec 31, 2024
When the underlying GitHub Client returns an error it gets swallowed
in this wrapper method. Not only that but also the emitted metrics are
wrong, as it counts one error AND success at the same time.

We've found this @grafana when using Atlantis in a massive repository
with many changes per minute, and sometimes Atlantis leave a comment
saying it's automatically merging but then nothing happens. But
checking the logs, we've found the following error message:

    Unable to merge pull, error: merging pull request: PUT
    https://api.github.com/repos/grafana/redacted/pulls/666/merge: 405
    Base branch was modified. Review and try the merge again. []

And because the error is swallowed and `InstrumentedClient.MergePull`
returns `nil`, then `Automerger` fails to leave a comment saying
merging failed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants