FR: Decline pull request with comment on merge rejection

[dploeger]

Currently, the plugin shows the result of the checks on a pull request only when hovering or clicking the "Merge" button.

To add more transparency to this, a rejected merge should immediately comment and decline the pull request. This should be configurable.

[seletskiy]

@dploeger: unfortunately, Merge Checks are not run by Bitbucket when you push new commits to PR until someone will open PR page in UI, so we can't make proactive reaction and add comments.

[dploeger]

@seletskiy But wouldn't that be okay? The PR workflow requires the UI anyway and currently the rejection is also only calculated on the PR UI page.

I have actually done just that in my (unreleased and probably now deprecated) YAML check hook and am currently working on a PR for this.

[seletskiy]

@dploeger: isn't it somewhat contradictional to what pull requests are for? If I understand correctly, you want to immediately decline PR if it is not passing merge check? In that case PR author will have no chance to fix his PR because it will be instantly declined.

This kind of workflow can produce excessive amount of rejected PRs since authors may just create new PR just to fix their previous PR, and this can be repeated several times in order to get passing PR.

Or authors can re-open declined PR, but it will too introduce unnecessary actions from their side. Also, how do you see logic flow in this case? Plugin declines PR, author re-opens it by pressing button in UI. Should plugin again immediately decline it?

Another problem there is that totally valid PR with on-going discussion can be randomly declined by WIP commit from author.

It's still not clear for me how do you see this feature, so I will be glad to hear you thoughts about things above.

[dploeger]

@seletskiy I would suggest a workflow like this:

• PR author opens PR
• Code checks fail, PR is declined
• PR author fixes the errors, reopens PR
• Code checks still fail, PR is declined again, Check, Reopen (repeat)
• Code checks succeed, PR can be merged

Comments aren't removed when a PR is declined and new comments can still be added. The PR author just has to reopen the PR once he is sure, that the code checks (should) succeed. This is only a click on the UI.

IMHO this makes it more visible to everybody, that the checks failed.

I would make this configurable, though.

What I would add without a configuration option, however, would be that each rejection or success will be added as a comment. This enables some kind of check history in the PR, which could become useful in a code review process.

What do you think?

[seletskiy]

@dploeger: yes, adding a message directly to PR makes sense, but decline is not. Maybe just having message is enough, why decline PR if it will be re-opened by author anyway?

It's quite a big change into Merge Check code, I think.

Technically, it's not so easy task, since we need to:

• store check state somewhere to not spam single PR with duplicate messages; not, that plugin does not have any kind of state right now;
• somehow pass message from underlying check back to Java Hook code; we can't use same Merge Check message actually because we can't use comment style formatting in here and this messsage can be quite big; we can pass it however via some kind of temporary file which name is given from Java code via environment variables;.

Do you feel that you can create Pull Request for this feature?

[dploeger]

@seletskiy I'm already on it. 😄

I just found this nasty bug, which was fixed in Bitbucket Server 5.7.1, that ignored my comment- and decline-requests. And I was debugging that last hour. 🙄

About the decline-feature: Would you be okay, to have it as a configurable feature which is disabled by default? I really would like to have that because of the better error visibility.

[seletskiy]

@dploeger:

About the decline-feature: Would you be okay, to have it as a configurable feature which is disabled by default? I really would like to have that because of the better error visibility.

Sure, if you find this useful, why not. I'm fine with configuration option.

[dploeger]

@seletskiy Argh, I thought a PullRequest could actually store properties, but it only has support for getProperties. This could've been a way to store the "I have already added a comment for this check" status.

I hadn't done this in my plugin, because I always decline and then no checks are run. 😉

Hm. Gotta find another way then.

[dploeger]

@seletskiy So the only method I can find is to add a reference to the version to a comment and scan the comments for that before adding the same comment again. Any other ideas?

[seletskiy]

@dploeger: maybe try to use https://developer.atlassian.com/server/framework/atlassian-sdk/store-and-retrieve-plugin-data/

[dploeger]

@seletskiy Well, yes. That or an ActiveObjects solution would also work, but that would mean that we'd have to manage that data (i.e. remove it, when a pull request gets deleted).