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

autoupdate: fix: status of previous HEAD commit is evaluated after base branch update #109

Merged
merged 6 commits into from
Mar 13, 2024

Conversation

fho
Copy link
Contributor

@fho fho commented Mar 13, 2024

When a Pull-Request was updated with it's base branch by the autoupdater, the GitHub Check status of the previous HEAD commit was evaluated.
If the check status for it was negative, the PR was suspended wrongly.
This PR fixes the issue.

The autoupdater now also logs a warning message if a check status for a different commit then the current HEAD commit returned by the update operation was retrieved.
This could happen if the branch is updated between the both API operations or GitHub returned old information (which can happen).
The autoupdater is not able to handle the situation correctly.

fho added 6 commits March 13, 2024 17:47
Instead of logging the ready for merge status only in
resumeIfPRMergeStatusPositive, always log it with a debug message when it
succeeds.

The review decision, ci_statuses and ci status summary fields are also not added
to the logger anymore in updatePR() and included in all further log messages.
It is unnecessary to include it in multiple log messages.
Change GithubClient.UpdateBranch to return the commit ID for that the update was
triggered.
If it the branch is already uptodate it returns the current head commit.
If the status is fetched for a different commit then the current HEAD commit,
log warning message.
This might happen when the GitHub API returns outdated information, or the PR
branch changed between both API calls.

Goordinator is currently not handling the situation. It can cause that a PR is
suspended wrongly because of the status of a previous commit.
If an update of a PR with it's base branch is scheduled and UpdateBranch is
called again until it returns that the branch is uptodate, updatePRWithBase
returns wrongly that the PR did not change.

This causes that updatePR() continues to run with the ready for
merge status of the previous HEAD commit.
If that one had failed checks it causes the PR to be suspended.

Fix it by returning changed=true if updatePRWithBase scheduled a update or the
branch was updated immediately via the github client.
- add set.Set.Contains() helper method
- replace usages of map[string]struct{} with our set.Set type
- replace cpBranchNames() with maps.Clone()
@fho fho self-assigned this Mar 13, 2024
@fho fho marked this pull request as ready for review March 13, 2024 17:01
@fho fho merged commit 91b9616 into main Mar 13, 2024
5 checks passed
@fho fho deleted the update_race branch March 13, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant