From ec5d60d027f225b2383d502aedf449dfb65fc199 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 3 Sep 2018 13:55:39 +0200 Subject: [PATCH] [CHG] runbot_merge: unapprove on PR update After discussion with mat, rco and moc, if a PR is updated it should be unapproved for safety reasons: if a reviewer approves a PR, that's what should be merged, if there are things to fix/change a reviewer should at least rubberstamp the changes to avoid mistakes. This is a bit more noisy/constraining, but can be changed or tuned afterwards if it's considered too constraining. --- runbot_merge/controllers/__init__.py | 7 +++---- runbot_merge/tests/test_basic.py | 12 +++++------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 4950da1a..0de09319 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -149,15 +149,14 @@ def handle_pr(env, event): _logger.error("Tentative sync to closed PR %s:%s", repo.name, pr['number']) return "It's my understanding that closed/merged PRs don't get sync'd" - if pr_obj.state == 'validated': - pr_obj.state = 'opened' - elif pr_obj.state == 'ready': - pr_obj.state = 'approved' + if pr_obj.state == 'ready': pr_obj.staging_id.cancel( "Updated PR %s:%s, removing staging %s", pr_obj.repository.name, pr_obj.number, pr_obj.staging_id, ) + if pr_obj.state != 'error': + pr_obj.state = 'opened' pr_obj.head = pr['head']['sha'] pr_obj.squash = pr['commits'] == 1 diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 0e5543cb..28a993f3 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -931,10 +931,10 @@ class TestPRUpdate(object): c2 = repo.make_commit(c, 'first', None, tree={'m': 'cc'}) prx.push(c2) assert pr.head == c2 - assert pr.state == 'approved' + assert pr.state == 'opened' def test_update_ready(self, env, repo): - """ Should reset to approved + """ Should reset to opened """ m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) repo.make_ref('heads/master', m) @@ -954,10 +954,10 @@ class TestPRUpdate(object): c2 = repo.make_commit(c, 'first', None, tree={'m': 'cc'}) prx.push(c2) assert pr.head == c2 - assert pr.state == 'approved' + assert pr.state == 'opened' def test_update_staged(self, env, repo): - """ Should cancel the staging & reset PR to approved + """ Should cancel the staging & reset PR to opened """ m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) repo.make_ref('heads/master', m) @@ -978,13 +978,11 @@ class TestPRUpdate(object): c2 = repo.make_commit(c, 'first', None, tree={'m': 'cc'}) prx.push(c2) assert pr.head == c2 - assert pr.state == 'approved' + assert pr.state == 'opened' assert not pr.staging_id assert not env['runbot_merge.stagings'].search([]) def test_update_error(self, env, repo): - """ Should cancel the staging & reset PR to approved - """ m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) repo.make_ref('heads/master', m)