From 679d556c90b8a0f83c462e74997621b40726c5e4 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 29 Nov 2024 09:04:06 +0100 Subject: [PATCH] [FIX] project creation: handling of mergebot info - don't *fail* in `_compute_identity`, it causes issues when the token is valid but doesn't have `user:email` access as the request is aborted and saving doesn't work - make `github_name` and `github_email` required rather than ad-hoc requiring them in `_compute_identity` (which doesn't work correctly) - force copy of `github_name` and `github_email`, with o2ms being !copy this means duplicating projects now works out of the box (or should...) Currently errors in `_compute_identity` are reported via logging which is not great as it's not UI visible, should probably get moved to chatter eventually but that's not currently enabled on projects. Fixes #990 --- conftest.py | 3 ++- forwardport/tests/test_conflicts.py | 4 ++++ forwardport/tests/test_limit.py | 2 ++ forwardport/tests/test_updates.py | 2 ++ forwardport/tests/test_weird.py | 4 ++++ mergebot_test_utils/utils.py | 2 ++ runbot_merge/models/project.py | 23 ++++++++------------- runbot_merge/tests/conftest.py | 2 ++ runbot_merge/tests/test_multirepo.py | 6 ++++++ runbot_merge/tests/test_oddities.py | 2 ++ runbot_merge/tests/test_status_overrides.py | 1 + 11 files changed, 36 insertions(+), 15 deletions(-) diff --git a/conftest.py b/conftest.py index 73a07eb4..de4848e9 100644 --- a/conftest.py +++ b/conftest.py @@ -179,7 +179,8 @@ def rolemap(request, config): r.raise_for_status() user = rolemap[role] = r.json() - data['user'] = user['login'] + n = data['user'] = user['login'] + data.setdefault('name', n) return rolemap @pytest.fixture diff --git a/forwardport/tests/test_conflicts.py b/forwardport/tests/test_conflicts.py index f6e23bbf..c56db8d6 100644 --- a/forwardport/tests/test_conflicts.py +++ b/forwardport/tests/test_conflicts.py @@ -193,6 +193,8 @@ def test_massive_conflict(env, config, make_repo): 'name': "thing", 'github_token': config['github']['token'], 'github_prefix': 'hansen', + 'github_name': config['github']['name'], + 'github_email': "foo@example.org", 'fp_github_token': config['github']['token'], 'fp_github_name': 'herbert', 'branch_ids': [ @@ -357,6 +359,8 @@ def test_conflict_deleted_deep(env, config, make_repo): 'name': "test", 'github_token': config['github']['token'], 'github_prefix': 'hansen', + 'github_name': config['github']['name'], + 'github_email': "foo@example.org", 'fp_github_token': config['github']['token'], 'fp_github_name': 'herbert', 'branch_ids': [ diff --git a/forwardport/tests/test_limit.py b/forwardport/tests/test_limit.py index 7df9d9e5..a7e74966 100644 --- a/forwardport/tests/test_limit.py +++ b/forwardport/tests/test_limit.py @@ -543,6 +543,8 @@ def post_merge(env, config, users, make_repo, branches): 'name': prod.name, 'github_token': config['github']['token'], 'github_prefix': 'hansen', + 'github_name': config['github']['name'], + 'github_email': "foo@example.org", 'fp_github_token': config['github']['token'], 'fp_github_name': 'herbert', 'branch_ids': [ diff --git a/forwardport/tests/test_updates.py b/forwardport/tests/test_updates.py index b13cd16a..4bd40bf9 100644 --- a/forwardport/tests/test_updates.py +++ b/forwardport/tests/test_updates.py @@ -320,6 +320,8 @@ def test_duplicate_fw(env, make_repo, setreviewers, config, users): 'name': 'a project', 'github_token': config['github']['token'], 'github_prefix': 'hansen', + 'github_name': config['github']['name'], + 'github_email': "foo@example.org", 'fp_github_token': config['github']['token'], 'fp_github_name': 'herbert', 'branch_ids': [ diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index 9577f690..a6439b78 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -242,6 +242,8 @@ class TestNotAllBranches: 'name': 'proj', 'github_token': config['github']['token'], 'github_prefix': 'hansen', + 'github_name': config['github']['name'], + 'github_email': "foo@example.org", 'fp_github_token': config['github']['token'], 'fp_github_name': 'herbert', 'branch_ids': [ @@ -1089,6 +1091,8 @@ def test_disable_multitudes(env, config, make_repo, users, setreviewers): "name": "bob", "github_token": config['github']['token'], "github_prefix": "hansen", + 'github_name': config['github']['name'], + 'github_email': "foo@example.org", "fp_github_token": config['github']['token'], "fp_github_name": "herbert", "branch_ids": [ diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py index 50976518..d936546e 100644 --- a/mergebot_test_utils/utils.py +++ b/mergebot_test_utils/utils.py @@ -122,6 +122,8 @@ def make_basic( 'name': project_name, 'github_token': config['github']['token'], 'github_prefix': 'hansen', + 'github_name': config['github']['name'], + 'github_email': "foo@example.org", 'fp_github_token': fp_token and config['github']['token'], 'fp_github_name': 'herbert', 'branch_ids': [ diff --git a/runbot_merge/models/project.py b/runbot_merge/models/project.py index c1d0c5d0..ec4dfc15 100644 --- a/runbot_merge/models/project.py +++ b/runbot_merge/models/project.py @@ -6,7 +6,6 @@ import requests import sentry_sdk from odoo import models, fields, api -from odoo.exceptions import UserError from odoo.osv import expression from odoo.tools import reverse_order @@ -41,8 +40,8 @@ class Project(models.Model): ) github_token = fields.Char("Github Token", required=True) - github_name = fields.Char(store=True, compute="_compute_identity") - github_email = fields.Char(store=True, compute="_compute_identity") + github_name = fields.Char(store=True, compute="_compute_identity", required=True, copy=True) + github_email = fields.Char(store=True, compute="_compute_identity", required=True, copy=True) github_prefix = fields.Char( required=True, default="hanson", # mergebot du bot du bot du~ @@ -74,11 +73,10 @@ class Project(models.Model): if not project.github_token or (project.github_name and project.github_email): continue - r0 = s.get('https://api.github.com/user', headers={ - 'Authorization': 'token %s' % project.github_token - }) + headers = {'Authorization': f'token {project.github_token}'} + r0 = s.get('https://api.github.com/user', headers=headers) if not r0.ok: - _logger.error("Failed to fetch merge bot information for project %s: %s", project.name, r0.text or r0.content) + _logger.warning("Failed to fetch merge bot information for project %s: %s", project.name, r0.text or r0.content) continue r = r0.json() @@ -88,20 +86,17 @@ class Project(models.Model): continue if 'user:email' not in set(re.split(r',\s*', r0.headers['x-oauth-scopes'])): - raise UserError("The merge bot github token needs the user:email scope to fetch the bot's identity.") - r1 = s.get('https://api.github.com/user/emails', headers={ - 'Authorization': 'token %s' % project.github_token - }) + _logger.warning("Unable to fetch merge bot emails for project %s: scope missing from token", project.name) + r1 = s.get('https://api.github.com/user/emails', headers=headers) if not r1.ok: - _logger.error("Failed to fetch merge bot emails for project %s: %s", project.name, r1.text or r1.content) + _logger.warning("Failed to fetch merge bot emails for project %s: %s", project.name, r1.text or r1.content) continue + project.github_email = next(( entry['email'] for entry in r1.json() if entry['primary'] ), None) - if not project.github_email: - raise UserError("The merge bot needs a public or accessible primary email set up.") # technically the email could change at any moment... @api.depends('fp_github_token') diff --git a/runbot_merge/tests/conftest.py b/runbot_merge/tests/conftest.py index 17b1546e..c3f8f249 100644 --- a/runbot_merge/tests/conftest.py +++ b/runbot_merge/tests/conftest.py @@ -10,6 +10,8 @@ def project(env, config): 'name': 'odoo', 'github_token': config['github']['token'], 'github_prefix': 'hansen', + 'github_name': config['github']['name'], + 'github_email': "foo@example.org", 'branch_ids': [(0, 0, {'name': 'master'})], }) diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 52e9bb2b..601dd4f5 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -939,6 +939,8 @@ class TestSubstitutions: p = env['runbot_merge.project'].create({ 'name': 'proj', 'github_token': 'wheeee', + 'github_name': "yyy", + 'github_email': "zzz@example.org", 'repo_ids': [(0, 0, {'name': 'xxx/xxx'})], 'branch_ids': [(0, 0, {'name': 'master'})] }) @@ -1058,6 +1060,8 @@ def test_multi_project(env, make_repo, setreviewers, users, config, 'name': 'Project 1', 'github_token': gh_token, 'github_prefix': 'hansen', + 'github_name': config['github']['name'], + 'github_email': "foo@example.org", 'repo_ids': [(0, 0, { 'name': r1.name, 'group_id': False, @@ -1079,6 +1083,8 @@ def test_multi_project(env, make_repo, setreviewers, users, config, 'name': "Project 2", 'github_token': gh_token, 'github_prefix': 'hansen', + 'github_name': config['github']['name'], + 'github_email': "foo@example.org", 'repo_ids': [(0, 0, { 'name': r2.name, 'group_id': False, diff --git a/runbot_merge/tests/test_oddities.py b/runbot_merge/tests/test_oddities.py index 9e7cea7b..e093b0c7 100644 --- a/runbot_merge/tests/test_oddities.py +++ b/runbot_merge/tests/test_oddities.py @@ -37,6 +37,8 @@ def test_name_search(env): p = env['runbot_merge.project'].create({ 'name': 'proj', 'github_token': 'no', + 'github_name': "noo", + 'github_email': "nooo@example.org", }) b = env['runbot_merge.branch'].create({ 'name': 'target', diff --git a/runbot_merge/tests/test_status_overrides.py b/runbot_merge/tests/test_status_overrides.py index 0d13ac61..bc71a4e3 100644 --- a/runbot_merge/tests/test_status_overrides.py +++ b/runbot_merge/tests/test_status_overrides.py @@ -21,6 +21,7 @@ def test_finding(env): project = env['runbot_merge.project'].create({ 'name': 'test', 'github_token': 'xxx', 'github_prefix': 'no', + 'github_name': "xxx", 'github_email': "xxx@example.org", }) repo_1 = env['runbot_merge.repository'].create({'project_id': project.id, 'name': 'r1'}) repo_2 = env['runbot_merge.repository'].create({'project_id': project.id, 'name': 'r2'})