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

Make merge fail explicitly when an approved PR also has changes requested #656

Merged

Conversation

Projects
None yet
4 participants
@umamaistempo
Copy link
Contributor

commented Jun 5, 2019

Currently here at Tink AB, frequently the following situation happens:

PR

  • Person request changes
  • New commit fix changes
  • Gets approval and ask bors to merge
  • Bors never says anything and nothing seems to happen
  • Someone notices this and dismiss stale "changes requested"
  • Ask for bors to merge and it works wonders

This is also correlated to #561

This PR aims to solve this problem by getting bors to explicitly refuse (providing message) when a merge is requested but there are changes requested.

Some minor types were added (as I was using them to explore the code to find out where the problem was). They can be merged as I intend to provide more typespecs in following PRs

Edit:
Assertions to test:

  • has approvals, has disapprovals, should fail and return msg
  • has approvals, has disapprovals, disapproval ignored in toml, should succeed
  • no approval, has disapprovals, disapproval ignored in toml, should succeed

Edit2:
There's no semantics for "ignore disapprovals", the only configs seems to be either require approvals (at which time disapprovals are considered) or doesn't (where disapprovals are also ignored).

This semantics seems strange TBH. I'll keep them working but I believe that the possible branchs should be

  • Required Approvals(int, 0 by default)
  • Ignore disapprovals(bool, false by default)

So a 0 approval, 1 disapproval when approvals are not required does not seem to make sense in the usual case

@umamaistempo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Considering returning different message for this case. Something along "Branch can't be merged because it has an open request for changes". What do you think ?

Minor improvement to the readability of BorsNG.Worker.Batcher.are_rev…
…iews_passing/2 to allow future improvements

@umamaistempo umamaistempo force-pushed the umamaistempo:fix-mergefail-when-changerequested branch from 50e7228 to c424623 Jun 5, 2019

end
passed_review = repo_conn
|> GitHub.get_reviews!(patch.pr_xref)
|> are_reviews_passing(toml)

This comment has been minimized.

Copy link
@umamaistempo

umamaistempo Jun 5, 2019

Author Contributor

Note: I wrote this way to keep with the style, but personally I think that it's more readable when we indent it in the next block:

review_status = 
  repo_conn
  |> GitHub.get_reviews!(patch.pr_xref)
  |> are_reviews_passing(toml)

This comment has been minimized.

Copy link
@notriddle

notriddle Jun 5, 2019

Member

This codebase predates mix format. Once we're up-to-date with the latest Elixir and OTP, I'll probably switch us over to following that convention.

@@ -5,23 +5,23 @@ defmodule BorsNG.Worker.Batcher.State do
and emits a batch state.
"""

@typep n :: BorsNG.Database.Status.state_n
@typep t :: BorsNG.Database.Status.state
@typep status :: BorsNG.Database.Status.t

This comment has been minimized.

Copy link
@umamaistempo

umamaistempo Jun 5, 2019

Author Contributor

this one was incorrect. state_n is how the state was being stored in the DB. The thing that the function receives is actually a list of %Status{} as can be seen

failed? = failed > 0
cond do
not required? ->
:sufficient

This comment has been minimized.

Copy link
@umamaistempo

umamaistempo Jun 5, 2019

Author Contributor

I really want to remove this. I included this clause to keep backwards compatibility (if required_approvals is not present, we don't check for failure) but I personally find this strange

This comment has been minimized.

Copy link
@notriddle

notriddle Jun 5, 2019

Member

Other users asked for it. @matklad

I think we could get away with removing that case in favor of having a bors r+ force command that would bypass branch reviews, but that should go through an RFC first.

This comment has been minimized.

Copy link
@matklad

matklad Jun 5, 2019

Yeah, I would prefer that there be an option to treat bors commands as a single source for whether the PR should be merged. At least in small-scale open-source projects, dismissing requests for changes seems like a busy work. It seems quite unlikely the r+er accidentally misses request for changes, and it is quite probably that they want to deliberately ignore it without extra clicks.

@spec are_reviews_passing(map, Batcher.BorsToml.t) :: :sufficient | :failed | :insufficient
defp are_reviews_passing(reviews, toml) do
%{"CHANGES_REQUESTED" => failed, "APPROVED" => approvals} = reviews
required? = is_integer(toml.required_approvals)

This comment has been minimized.

Copy link
@umamaistempo

umamaistempo Jun 5, 2019

Author Contributor

considering renaming to review_required?

This comment has been minimized.

Copy link
@notriddle

notriddle Jun 5, 2019

Member

Changing it to review_required? sounds good to me.

{_, approved} when approved >= required -> :sufficient
{0, _} -> :insufficient
@spec are_reviews_passing(map, Batcher.BorsToml.t) :: :sufficient | :failed | :insufficient
defp are_reviews_passing(reviews, toml) do

This comment has been minimized.

Copy link
@umamaistempo

umamaistempo Jun 5, 2019

Author Contributor

considering renaming to reviews_status as the current function name sounds like something that returns a boolean

This comment has been minimized.

Copy link
@notriddle

notriddle Jun 5, 2019

Member

Changing it to reviews_status sounds good to me.

@@ -383,6 +383,7 @@ defmodule BorsNG.Worker.Batcher do
end
end

@spec complete_batch(Status.state, Batch.t, [Status.t]) :: :ok

This comment has been minimized.

Copy link
@umamaistempo

umamaistempo Jun 5, 2019

Author Contributor

I did not explore the code enought to find out if by design it is impossible for waiting status batch to hit the maybe_complete_batch/1 fun; if there's no safety net there, then it's possible for this function to receive such state and that would raise a function argument error (we're only treating the cases of ok, error and running

This comment has been minimized.

Copy link
@notriddle

notriddle Jun 5, 2019

Member

It should be impossible to reach complete_batch while still in the :waiting state.

When it's in a :waiting state, it means it hasn't created a staging branch commit. Reaching maybe_complete_batch/1 while still in the :waiting stage means the batch received a build status before it actually had a commit to build.

This comment has been minimized.

Copy link
@umamaistempo

umamaistempo Jun 7, 2019

Author Contributor

So, I will merge this PR now, but then, to improve documentation, I'll make the type

@type staged_state :: :ok | :error | :running

So we can specify it here, what do you think ? :^)

@notriddle
Copy link
Member

left a comment

I have a couple of questions and comments, but the code looks good.

The goal of this feature, as you've hopefully intuited, is to duplicate the behavior of GitHub Branch Protection. Making it follow GitHub's own behavior more closely is considered a bugfix.

bors delegate+

@@ -383,6 +383,7 @@ defmodule BorsNG.Worker.Batcher do
end
end

@spec complete_batch(Status.state, Batch.t, [Status.t]) :: :ok

This comment has been minimized.

Copy link
@notriddle

notriddle Jun 5, 2019

Member

It should be impossible to reach complete_batch while still in the :waiting state.

When it's in a :waiting state, it means it hasn't created a staging branch commit. Reaching maybe_complete_batch/1 while still in the :waiting stage means the batch received a build status before it actually had a commit to build.

end
passed_review = repo_conn
|> GitHub.get_reviews!(patch.pr_xref)
|> are_reviews_passing(toml)

This comment has been minimized.

Copy link
@notriddle

notriddle Jun 5, 2019

Member

This codebase predates mix format. Once we're up-to-date with the latest Elixir and OTP, I'll probably switch us over to following that convention.

{_, approved} when approved >= required -> :sufficient
{0, _} -> :insufficient
@spec are_reviews_passing(map, Batcher.BorsToml.t) :: :sufficient | :failed | :insufficient
defp are_reviews_passing(reviews, toml) do

This comment has been minimized.

Copy link
@notriddle

notriddle Jun 5, 2019

Member

Changing it to reviews_status sounds good to me.

@spec are_reviews_passing(map, Batcher.BorsToml.t) :: :sufficient | :failed | :insufficient
defp are_reviews_passing(reviews, toml) do
%{"CHANGES_REQUESTED" => failed, "APPROVED" => approvals} = reviews
required? = is_integer(toml.required_approvals)

This comment has been minimized.

Copy link
@notriddle

notriddle Jun 5, 2019

Member

Changing it to review_required? sounds good to me.

failed? = failed > 0
cond do
not required? ->
:sufficient

This comment has been minimized.

Copy link
@notriddle

notriddle Jun 5, 2019

Member

Other users asked for it. @matklad

I think we could get away with removing that case in favor of having a bors r+ force command that would bypass branch reviews, but that should go through an RFC first.

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

✌️ umamaistempo can now approve this pull request

@ForsakenHarmony

This comment has been minimized.

Copy link

commented Jun 5, 2019

does this also work if the master branch is protected, but there's no reviews at all?

@umamaistempo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

bors r+

bors bot added a commit that referenced this pull request Jun 7, 2019

Merge #656
656: Make merge fail explicitly when an approved PR also has changes requested r=umamaistempo a=umamaistempo

Currently here at Tink AB, frequently the following situation happens:

PR
- Person request changes
- New commit fix changes
- Gets approval and ask bors to merge
- Bors never says anything and nothing seems to happen
- Someone notices this and dismiss stale "changes requested"
- Ask for bors to merge and it works wonders

This is also correlated to #561 

This PR aims to solve this problem by getting bors to explicitly refuse (providing message) when a merge is requested but there are changes requested.

Some minor types were added (as I was using them to explore the code to find out where the problem was). They can be merged as I intend to provide more typespecs in following PRs

Edit:
Assertions to test:
- [x] has approvals, has disapprovals, should fail and return msg
- [ ] ~has approvals, has disapprovals, disapproval ignored in toml, should succeed~
- [ ] ~no approval, has disapprovals, disapproval ignored in toml, should succeed~

Edit2:
There's no semantics for "ignore disapprovals", the only configs seems to be either require approvals (at which time disapprovals are considered) or doesn't (where disapprovals are also ignored).

This semantics seems strange TBH. I'll keep them working but I believe that the possible branchs should be

- Required Approvals(int, 0 by default)
- Ignore disapprovals(bool, false by default)

So a 0 approval, 1 disapproval when approvals are not required does not seem to make sense in the usual case

Co-authored-by: Charlotte Lorelei de Oliveira <11341349+umamaistempo@users.noreply.github.com>
@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Build succeeded

@bors bors bot merged commit c7129d2 into bors-ng:master Jun 7, 2019

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
bors Build succeeded
Details

@umamaistempo umamaistempo deleted the umamaistempo:fix-mergefail-when-changerequested branch Jun 7, 2019

pull bot pushed a commit to Mattlk13/bors-ng that referenced this pull request Jun 14, 2019

Merge bors-ng#648
648: Bump distillery from 2.0.12 to 2.0.14 r=notriddle a=dependabot-preview[bot]

Bumps [distillery](https://github.com/bitwalker/distillery) from 2.0.12 to 2.0.14.
<details>
<summary>Commits</summary>

- [`487875d`](bitwalker/distillery@487875d) Version 2.0.14
- [`3f1f00d`](bitwalker/distillery@3f1f00d) Use npm to build JavaScript assets ([bors-ng#656](https://github-redirect.dependabot.com/bitwalker/distillery/issues/656))
- [`4776b59`](bitwalker/distillery@4776b59) Prevents generating cookies that contain commas ([bors-ng#663](https://github-redirect.dependabot.com/bitwalker/distillery/issues/663))
- [`47ac647`](bitwalker/distillery@47ac647) Retry a few times when testing ability to hit HTTP endpoint in integration test
- [`7963492`](bitwalker/distillery@7963492) Fix ambiguous redirect error
- [`a7769f1`](bitwalker/distillery@a7769f1) Do not run build for 1.6+OTP 22
- [`9b79043`](bitwalker/distillery@9b79043) Fixes for Travis build
- [`c0ee187`](bitwalker/distillery@c0ee187) Potential fix for [bors-ng#662](https://github-redirect.dependabot.com/bitwalker/distillery/issues/662)
- [`e2ff64b`](bitwalker/distillery@e2ff64b) Add release version to describe command ([bors-ng#636](https://github-redirect.dependabot.com/bitwalker/distillery/issues/636))
- [`16d3850`](bitwalker/distillery@16d3850) Fix typo in Release.get_code_paths/1 ([bors-ng#637](https://github-redirect.dependabot.com/bitwalker/distillery/issues/637))
- Additional commits viewable in [compare view](bitwalker/distillery@2.0.12...2.0.14)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=distillery&package-manager=hex&previous-version=2.0.12&new-version=2.0.14)](https://dependabot.com/compatibility-score.html?dependency-name=distillery&package-manager=hex&previous-version=2.0.12&new-version=2.0.14)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>

Co-authored-by: dependabot[bot] <support@dependabot.com>

notriddle added a commit to bors-ng/bors-ng.github.io that referenced this pull request Jul 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.