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

1069: PR ends up in bad state if interrupted just after push #1183

Closed
wants to merge 3 commits into from

Conversation

erikj79
Copy link
Member

@erikj79 erikj79 commented Jun 7, 2021

This change adds a new transaction step when integrating a PR, to better handle if the bot gets interrupted mid integration. Currently, if the change is pushed and the bot is interrupted before closing, changing labels or adding the final "commit pushed" comment, the PR can end up in a limbo state.

The new step is another comment "Going to push commit as ..." which gets added right before the git push command is run. Using this comment, it's now possible to automatically recover if the bot gets interrupted. The integration command checks for any such comments and if found, checks if the commit hash is present in the target. If it is, the PR was already pushed, and the command will just close it out as normal.

I also decided to move the output of any rebase command to this prepush comment, so we don't risk losing it.


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Issue

  • SKARA-1069: PR ends up in bad state if interrupted just after push

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/skara pull/1183/head:pull/1183
$ git checkout pull/1183

Update a local copy of the PR:
$ git checkout pull/1183
$ git pull https://git.openjdk.java.net/skara pull/1183/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1183

View PR using the GUI difftool:
$ git pr show -t 1183

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/skara/pull/1183.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 7, 2021

👋 Welcome back erikj! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot changed the title SKARA-1069 1069: PR ends up in bad state if interrupted just after push Jun 7, 2021
@openjdk openjdk bot added the rfr label Jun 7, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 7, 2021

Webrevs

Copy link
Member

@kevinrushforth kevinrushforth left a comment

It looks OK to me. I added a couple inline questions.

@@ -196,20 +229,9 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
// Rebase and push it!
if (!localHash.equals(PullRequestUtils.targetHash(pr, localRepo))) {
var amendedHash = checkablePr.amendManualReviewers(localHash, censusInstance.namespace(), original);
addPrePushComment(pr, amendedHash, rebaseMessage.toString());
localRepo.push(amendedHash, pr.repository().url(), pr.targetRef());
Copy link
Member

@kevinrushforth kevinrushforth Jun 7, 2021

Choose a reason for hiding this comment

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

If the push fails and the bot dies, will it still be retried after this fix?

Copy link
Member Author

@erikj79 erikj79 Jun 8, 2021

Choose a reason for hiding this comment

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

Yes, that's the idea at least, but I should write a test for it. If the push fails, the next time around, the prepush comment is found, but the hash is not found in the target, so the IntegrationCommand will continue as if nothing has happened.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 7, 2021

@erikj79 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

🔍 One or more changes in this pull request modifies files in areas of the source code that often require two reviewers. Please consider if this is the case for this pull request, and if so, await a second reviewer to approve this pull request before you integrate it.

After integration, the commit message for the final commit will be:

1069: PR ends up in bad state if interrupted just after push

Reviewed-by: kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jun 7, 2021
@erikj79
Copy link
Member Author

@erikj79 erikj79 commented Jun 9, 2021

Adding new patch addressing review comments. Reverted the allComments change. Added testing of interruption between posting the prepush comment and doing the push.

The big change here is I discovered the SponsorCommand had a lot of duplicate code that needed to be updated in the same way. The Skara codebase seem to shun inheritance to share logic, so I tried to just move some things to static methods on IntegrateCommand and have SponsorCommand call them when suitable. There are more duplicates that could be given the same treatment, but I tried to limit the changes here to what was relevant for this fix. I'm thinking a super class could help a lot here in a future change.

I also duplicated the main test I wrote from IntegrateTests to SponsorTests to verify the same behaviors since we have two almost identical code paths for the same thing.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good as far as I can tell.

@erikj79
Copy link
Member Author

@erikj79 erikj79 commented Jun 9, 2021

/integrate

@openjdk openjdk bot closed this Jun 9, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 9, 2021

@erikj79 Pushed as commit 3091bee.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants