Build Status and Comment Timing

Hi,

I’m currently working on a plugin that will block a merge from occurring based on the build status as well if an automated comment gets posted to the pull request. I currently implement RepositoryMergeCheck and use PreRepositoryHook’s preUpdate method to get the build status whenever a new one is posted. In addition to that I’m using the PullRequestService to get the latest comment that was posted to the pull request.

The issue I’m finding is that there could potentially be a timing issue between when the build status was posted and when the comment was posted. For example if the build status turns out to be OK and then it checks for the latest comment (which still hasn’t been posted) and it can’t find anything it will allow the merge to occur however eventually a comment gets posted which means it should have blocked the merge.

I’ve tried to add an EventListener and that works great to let me know any time a comment is posted but how can I then block the merge from inside the EventListener? The only parameter it takes is PullRequestCommentAddedEvent.

If I could please get some help as to how to solve this timing issue so that I can check whether or not I should block a merge only after both the build status and a potential comment has been posted to a pull request.

Thank You.

Hi @O.N,
My recommendation to you is to change the workflow to make the build fail if the pull request should not be merged. The comment can still be added as context for the author to know what needs to be fixed. But if the build produced information indicating that something needs to change before the branch can be merged, I’d consider it a failing build. Comments are not meant to be used to block a pull request. Tasks maybe. But still, the 2 step process you are trying to put in place I fear will always give you a a headache.

That being said, considering what you are trying to achieve, would something like this work?

  • Every time the build completes, a comment is posted including the commit ID that was built (i.e.: the HEAD of the pull request’s fromRef available in the PullRequestMergeHookRequest). The comment also says whether the PR can be merged
  • In the RepositoryMergeCheck you look at all comments in search for one including the commit ID and the status to make the decision. The absence of a comment means the build has not posted the status and so the PR should be blocked until you have a complete analysis.

NOTE: this approach will add a lot of comments to the PR, it will make the activity stream very verbose, and it will make the emails a bit spammy. In larger PRs the merge check may even take a bit longer (though not sure if it will be noticeable) since it will need to (potentially) run through a lot more comments before coming to a decision.

Hope that helps
Juan Palacios

1 Like

Hi Juan,

Thank you for your reply. In response to your suggestion, the 3rd party plugin we’re currently using does something very similar to what you’re suggesting. Every time the build completes they send the status of the build as well it posts a comment containing information regarding any errors that were found, these errors relate to syntax in the code and not necessarily the build itself. The issue I’m concerned with is if in this approach (similar to what you suggested) what happens if the build is posted so it searches the comment and it can’t find one with the commit ID, it will block the merge however right after the search is completed the comment gets posted. In this scenario the merge gets blocked because the comment wasn’t posted in time before searching for it.

My main concern is the time it takes between when the build gets posted and when the comment gets posted. The comment could potentially be posted only after the search has been concluded thus leading to a false positive.

Is there any way to access the RepositoryHookResult or something similar that can block the merge inside the EventListener?

Thank You

Hi @O.N
Can you please provide a little bit more context? How are you trying to trigger the merge? Is it from the UI?

The merge checks are executed (with the dryRun flag set to true) every time the pull request page is loaded. So it doesn’t matter when the comment is posted. Once that happens the MERGE button will be enabled and the merge should complete successfully.

Cheers
Juan Palacios

Hi @jpalacios

I’ll try to add a bit more context. Currently the process we’re trying to put in place is the following:

  1. Every time a pull request is created or a new commit is added to a pull request some 3rd party software will run an analysis on the commit.

  2. Once the analysis is completed the 3rd party will send the following back to the pull request: A build status indicating whether or not the build was a successful; A comment if any syntax errors were found or fixed, if nothing was found or fixed NO comment will be produced.

  3. Our plugin will wait to hear back from the 3rd party and if the build fails or if a comment was posted indicating that any errors were found our plugin will block the merge (disabling the merge button).

You mentioned something interesting:

The merge checks are executed (with the dryRun flag set to true) every time the pull request page is loaded.

I have a couple of questions regarding that. Does that mean that RepositoryHookResult preUpdate() runs every time the pull request page is loaded? Is that by default or do we have to set that dryRun flag to true somewhere? If the user never reloads the page the will the build status not show the latest until the page is reloaded?

Thanks for your help on this @jpalacios

Hi @O.N

Thanks for providing more context. It sounds like you have a bit of an “undetermined” state there were a build succeeds and there’s no comment but you have no way of knowing if it’s because no comment is going to be posted, or because it’s simply delayed.

Did you consider my suggestion of having the build fail when the plugin finds things that need to be fixed? IS that something you can configure? Then you could use the existing required build merge checks to prevent the pull requests from merging.

In answer to your questions:

Does that mean that RepositoryHookResult preUpdate() runs every time the pull request page is loaded? Is that by default or do we have to set that dryRun flag to true somewhere?

That behaviour is by default. You don’t have to set any flags. The system will set the dryRun flag to true in the request object (see PullRequestMergeHookRequest which extends RepositoryHookRequest) when calling the RepositoryMergeCheck so that hook implementations can distinguish, if they need to, between a merge check that happens as part of processing a merge request and a merge check that happens just to inform the user of the pull request’s merge-ability state.

If the user never reloads the page the will the build status not show the latest until the page is reloaded?

Correct. However, even if the merge button is in a stale enabled state, when the user clicks it the system will run all necessary checks to make sure that a pull request is not merged when it shouldn’t be.

Regards
Juan Palacios

Hi @jpalacios

Unfortunately we don’t know if the plugin found any errors until a comment is posted (or not posted) so having the build fail isn’t an option for us since we have to wait for the comment.

Since there’s no way right now to block a merge when a comment is added to a pull request it seems there’s no way to guarantee that this timing scenario won’t happen, albeit not highly probable.

Thanks for your help on this @jpalacios and if there’s anything else you could think of I’d love to hear it.

I’m sorry I wasn’t able to find a complete solution for you. If I can think of anything else I’ll let you know.

Cheers
Juan Palacios

Hi @jpalacios

Had another idea presented to me by a co-worker, but was wondering if there’s a way to get all of the build statuses of a pull request. The idea is to count the amount build status that were posted vs the amount of comments posted (with some additional logic) to try and determine if the latest comment was posted.

Was wondering whats the fastest way to get all of the posted build status of the pull request.

Thanks.

Hi @O.N,
I believe what you’re looking for is the BuildStatusService#getSummary API. It should give you a breakdown of successful, failed and in-progress builds. Alternatively you can use BuildStatusService#findAll which returns BuildStatus entities with a bit more detail.

Hope this helps you get your workflow going.

Regards
Juan Palacios

Thank you @jpalacios. Was wondering if there is a way to get all of the build status in 1 single call? I noticed those methods take a commitId but I worry that in a long running pull request that can take quite some time. If there’s a way to get all of the build status of the pull request at the same time that would be faster I think.

Hi @O.N,
I don’t think that will be possible. A build status is associated with a commit ID not a pull request. To us it makes sense to keep these concepts loosely coupled since a commit can be part of any number of branches (and PR’s) so querying by commit ID means one build status can be displayed in multiple locations (PRs, branch list, commit list, commit page).

In your earlier comment you said:

The idea is to count the amount build status that were posted vs the amount of comments posted (with some additional logic) to try and determine if the latest comment was posted.

I’m not sure what the “additional logic” is but, wouldn’t this still have a timing issue? The count could still be off either because no issue was found and no comment was posted or because the latest comment hasn’t arrived yet (also because an earlier build, or several, didn’t produce a comment). I’d expect older builds to be irrelevant. It comes down to whether there’s a build status for the HEAD of the fromRef and, if so, looking for a matching comment, right? If that’s the case the timing issue will always be present.

Have you considered establishing a “grace period” using BuildStatus#getDateAdded? It’s not the best solution, but perhaps you could put a rule in place which says: if the build status is present but no comment has been received after X minutes the pull request can be merged

Regards
Juan Palacios

Thanks for the reply @jpalacios, makes sense about the commit ID.

In thinking about the “grace period” idea I think the issue here is if the comment gets posted before the build status, then that means the pull request will be unblocked.

Hi @O.N
Absolutely, but you’d apply both rules is my point. Look for a comment matching the latests commit (I believe you said the commit ID was included in the comment?) and if you can’t find it check the latest build status’ dateAdded. If the grace period hasn’t completed then veto the merge. Or maybe I’m missing something?

Cheers
Juan Palacios

Hi @jpalacios, my mistake now I understand what you meant. If I did mention that the commitId is in the comment then I misspoke as it does not contain the commitId. Without having that connection between the comment and build I can’t see a way to connect the 2 in the scenario when a comment is posted.

Thanks