From 31170be1c70b5181b7e07917035340263cd17517 Mon Sep 17 00:00:00 2001 From: Christophe Monniez Date: Thu, 8 Dec 2022 14:50:34 +0100 Subject: [PATCH] [REF] runbot: remove log_path useless parameter In the same spirit as the previous commit, the log_path parameter is removed from _run_* methods in order to centralize its computation. Most of the time this parameter was left untouched and just passed as is to the next method. --- runbot/models/build.py | 4 +- runbot/models/build_config.py | 32 +++++++------- runbot/models/build_config_codeowner.py | 2 +- runbot/tests/test_build_config_step.py | 58 ++++++++++++------------- runbot_cla/build_config.py | 2 +- 5 files changed, 49 insertions(+), 49 deletions(-) diff --git a/runbot/models/build.py b/runbot/models/build.py index 3e5c42f2..f1060df2 100644 --- a/runbot/models/build.py +++ b/runbot/models/build.py @@ -641,8 +641,6 @@ class BuildResult(models.Model): build._log('wake_up', 'Impossible to wake-up, **build dir does not exists anymore**', log_type='markdown', level='SEPARATOR') else: try: - log_path = build._path('logs', 'wake_up.txt') - port = self._find_port() build.write({ 'job_start': now(), @@ -658,7 +656,7 @@ class BuildResult(models.Model): run_step = step_ids[-1] else: run_step = self.env.ref('runbot.runbot_build_config_step_run') - run_step._run_step(build, log_path, force=True) + run_step._run_step(build, force=True) # reload_nginx will be triggered by _run_run_odoo except Exception: _logger.exception('Failed to wake up build %s', build.dest) diff --git a/runbot/models/build_config.py b/runbot/models/build_config.py index 24fb47b0..a1d6eede 100644 --- a/runbot/models/build_config.py +++ b/runbot/models/build_config.py @@ -269,7 +269,6 @@ class ConfigStep(models.Model): raise UserError('Invalid extra_params on config step') def _run(self, build): - log_path = build._path('logs', '%s.txt' % self.name) build.write({'job_start': now(), 'job_end': False}) # state, ... log_link = '' if self._has_log(): @@ -277,13 +276,15 @@ class ConfigStep(models.Model): url = f"{log_url}/runbot/static/build/{build.dest}/logs/{self.name}.txt" log_link = f'[@icon-file-text]({url})' build._log('run', 'Starting step **%s** from config **%s** %s' % (self.name, build.params_id.config_id.name, log_link), log_type='markdown', level='SEPARATOR') - self._run_step(build, log_path) + self._run_step(build) - def _run_step(self, build, log_path, **kwargs): + def _run_step(self, build, **kwargs): build.log_counter = self.env['ir.config_parameter'].sudo().get_param('runbot.runbot_maxlogs', 100) run_method = getattr(self, '_run_%s' % self.job_type) - docker_params = run_method(build, log_path, **kwargs) + docker_params = run_method(build, **kwargs) if docker_params: + if 'log_path' not in docker_params: + docker_params['log_path'] = build._path('logs', f'{self.name}.txt') if 'cpu_limit' not in docker_params: max_timeout = int(self.env['ir.config_parameter'].get_param('runbot.runbot_timeout', default=10000)) docker_params['cpu_limit'] = min(self.cpu_limit, max_timeout) @@ -295,7 +296,7 @@ class ConfigStep(models.Model): docker_params['cpus'] = float((logical_cpu_count / physical_cpu_count) * container_cpus) build._docker_run(**docker_params) - def _run_create_build(self, build, log_path): + def _run_create_build(self, build): count = 0 config_data = build.params_id.config_data config_ids = config_data.get('create_config_ids', self.create_config_ids) @@ -332,7 +333,7 @@ class ConfigStep(models.Model): 'PatchSet': PatchSet, } - def _run_python(self, build, log_path): + def _run_python(self, build): eval_ctx = self.make_python_ctx(build) try: safe_eval(self.python_code.strip(), eval_ctx, mode="exec", nocopy=True) @@ -356,7 +357,7 @@ class ConfigStep(models.Model): self.ensure_one() return self.job_type in ('install_odoo', 'run_odoo', 'restore', 'test_upgrade') or (self.job_type == 'python' and ('docker_params =' in self.python_code or '_run_' in self.python_code)) - def _run_run_odoo(self, build, log_path, force=False): + def _run_run_odoo(self, build, force=False): if not force: if build.parent_id: build._log('_run_run_odoo', 'build has a parent, skip run') @@ -414,9 +415,10 @@ class ConfigStep(models.Model): self.env.cr.commit() # commit before docker run to be 100% sure that db state is consistent with dockers self.invalidate_cache() self.env['runbot.runbot']._reload_nginx() - return dict(cmd=cmd, log_path=log_path, container_name=docker_name, exposed_ports=[build_port, build_port + 1], ro_volumes=exports, env_variables=env_variables) + return dict(cmd=cmd, container_name=docker_name, exposed_ports=[build_port, build_port + 1], ro_volumes=exports, env_variables=env_variables) - def _run_install_odoo(self, build, log_path): + + def _run_install_odoo(self, build): exports = build._checkout() modules_to_install = self._modules_to_install(build) @@ -498,7 +500,7 @@ class ConfigStep(models.Model): cmd.finals.append(['flamegraph.pl', '--title', 'Flamegraph %s for build %s' % (self.name, build.id), self._perfs_data_path(), '>', self._perfs_data_path(ext='svg')]) cmd.finals.append(['gzip', '-f', self._perfs_data_path()]) # keep data but gz them to save disc space env_variables = self.additionnal_env.split(';') if self.additionnal_env else [] - return dict(cmd=cmd, log_path=log_path, container_name=build._get_docker_name(), ro_volumes=exports, env_variables=env_variables) + return dict(cmd=cmd, container_name=build._get_docker_name(), ro_volumes=exports, env_variables=env_variables) def _upgrade_create_childs(self): pass @@ -546,7 +548,7 @@ class ConfigStep(models.Model): ) child._log('', 'This build tests change of schema in stable version testing upgrade to %s' % target.params_id.version_id.name) - def _run_configure_upgrade(self, build, log_path): + def _run_configure_upgrade(self, build): """ Source/target parameters: - upgrade_to_current | (upgrade_to_master + (upgrade_to_major_versions | upgrade_to_all_versions)) @@ -708,7 +710,7 @@ class ConfigStep(models.Model): if any(fnmatch.fnmatch(db.db_suffix, pat) for pat in pat_list): yield db - def _run_test_upgrade(self, build, log_path): + def _run_test_upgrade(self, build): target = build.params_id.upgrade_to_build_id commit_ids = build.params_id.commit_ids target_commit_ids = target.params_id.commit_ids @@ -739,9 +741,9 @@ class ConfigStep(models.Model): exception_env = self.env['runbot.upgrade.exception']._generate() if exception_env: env_variables.append(exception_env) - return dict(cmd=migrate_cmd, log_path=log_path, container_name=build._get_docker_name(), ro_volumes=exports, env_variables=env_variables, image_tag=target.params_id.dockerfile_id.image_tag) + return dict(cmd=migrate_cmd, container_name=build._get_docker_name(), ro_volumes=exports, env_variables=env_variables, image_tag=target.params_id.dockerfile_id.image_tag) - def _run_restore(self, build, log_path): + def _run_restore(self, build): # exports = build._checkout() params = build.params_id @@ -782,7 +784,7 @@ class ConfigStep(models.Model): ]) - return dict(cmd=cmd, log_path=log_path, container_name=build._get_docker_name()) + return dict(cmd=cmd, container_name=build._get_docker_name()) def _reference_builds(self, bundle, trigger): upgrade_dumps_trigger_id = trigger.upgrade_dumps_trigger_id diff --git a/runbot/models/build_config_codeowner.py b/runbot/models/build_config_codeowner.py index 79da1cff..07b57391 100644 --- a/runbot/models/build_config_codeowner.py +++ b/runbot/models/build_config_codeowner.py @@ -60,7 +60,7 @@ class ConfigStep(models.Model): reviewer_per_file[file] = file_reviewers return reviewer_per_file - def _run_codeowner(self, build, log_path): + def _run_codeowner(self, build): bundle = build.params_id.create_batch_id.bundle_id if bundle.is_base: build._log('', 'Skipping base bundle') diff --git a/runbot/tests/test_build_config_step.py b/runbot/tests/test_build_config_step.py index 1053a098..61da1430 100644 --- a/runbot/tests/test_build_config_step.py +++ b/runbot/tests/test_build_config_step.py @@ -43,7 +43,7 @@ class TestCodeowner(TestBuildConfigStepCommon): def test_codeowner_is_base(self): self.dev_bundle.is_base = True - self.config_step._run_codeowner(self.parent_build, '/tmp/essai') + self.config_step._run_codeowner(self.parent_build) self.assertEqual(self.parent_build.log_ids.mapped('message'), [ 'Skipping base bundle', ]) @@ -52,7 +52,7 @@ class TestCodeowner(TestBuildConfigStepCommon): def test_codeowner_check_limits(self): self.parent_build.params_id.commit_link_ids[0].file_changed = 451 self.parent_build.params_id.commit_link_ids[0].base_ahead = 51 - self.config_step._run_codeowner(self.parent_build, '/tmp/essai') + self.config_step._run_codeowner(self.parent_build) self.assertEqual(self.parent_build.log_ids.mapped('message'), [ 'Limit reached: dfdfcfcf has more than 50 commit (51) and will be skipped. Contact runbot team to increase your limit if it was intended', 'Limit reached: dfdfcfcf has more than 450 modified files (451) and will be skipped. Contact runbot team to increase your limit if it was intended', @@ -60,8 +60,8 @@ class TestCodeowner(TestBuildConfigStepCommon): self.assertEqual(self.parent_build.local_result, 'ko') def test_codeowner_draft(self): - self.dev_pr.draft = True - self.config_step._run_codeowner(self.parent_build, '/tmp/essai') + self.dev_pr.draft=True + self.config_step._run_codeowner(self.parent_build) self.assertEqual(self.parent_build.log_ids.mapped('message'), [ 'Some pr are draft, skipping: 1234' ]) @@ -74,7 +74,7 @@ class TestCodeowner(TestBuildConfigStepCommon): def test_codeowner_forwardpot(self): self.dev_pr.pr_author = 'fw-bot' - self.config_step._run_codeowner(self.parent_build, '/tmp/essai') + self.config_step._run_codeowner(self.parent_build) self.assertEqual(self.parent_build.log_ids.mapped('message'), [ 'Ignoring forward port pull request: 1234' ]) @@ -82,7 +82,7 @@ class TestCodeowner(TestBuildConfigStepCommon): def test_codeowner_invalid_target(self): self.dev_pr.target_branch_name = 'master-other-dev-branch' - self.config_step._run_codeowner(self.parent_build, '/tmp/essai') + self.config_step._run_codeowner(self.parent_build) self.assertEqual(self.parent_build.log_ids.mapped('message'), [ 'Some pr have an invalid target: 1234' ]) @@ -98,7 +98,7 @@ class TestCodeowner(TestBuildConfigStepCommon): }) second_pr.pull_head_name = f'{self.remote_server.owner}:{self.dev_branch.name}' second_pr.bundle_id = self.dev_bundle.id - self.config_step._run_codeowner(self.parent_build, '/tmp/essai') + self.config_step._run_codeowner(self.parent_build) self.assertEqual(self.parent_build.log_ids.mapped('message'), [ "More than one open pr in this bundle for server: ['1234', '1235']" ]) @@ -114,7 +114,7 @@ class TestCodeowner(TestBuildConfigStepCommon): def test_codeowner_regex_multiple(self): self.diff = 'file.js\nfile.py\nfile.xml' - self.config_step._run_codeowner(self.parent_build, '/tmp/essai') + self.config_step._run_codeowner(self.parent_build) messages = self.parent_build.log_ids.mapped('message') self.assertEqual(messages[1], 'Checking 2 codeowner regexed on 3 files') self.assertEqual(messages[2], 'Adding team_js to reviewers for file [server/file.js](https://False/blob/dfdfcfcf/file.js)') @@ -126,14 +126,14 @@ class TestCodeowner(TestBuildConfigStepCommon): def test_codeowner_regex_some_already_on(self): self.diff = 'file.js\nfile.py\nfile.xml' self.dev_pr.reviewers = 'codeowner-team,team_js' - self.config_step._run_codeowner(self.parent_build, '/tmp/essai') + self.config_step._run_codeowner(self.parent_build) messages = self.parent_build.log_ids.mapped('message') self.assertEqual(messages[5], 'Requesting review for pull request [base/server:1234](https://example.com/base/server/pull/1234): team_py') def test_codeowner_regex_all_already_on(self): self.diff = 'file.js\nfile.py\nfile.xml' self.dev_pr.reviewers = 'codeowner-team,team_js,team_py' - self.config_step._run_codeowner(self.parent_build, '/tmp/essai') + self.config_step._run_codeowner(self.parent_build) messages = self.parent_build.log_ids.mapped('message') self.assertEqual(messages[5], 'All reviewers are already on pull request [base/server:1234](https://example.com/base/server/pull/1234)') @@ -143,7 +143,7 @@ class TestCodeowner(TestBuildConfigStepCommon): self.team1.github_logins = 'some_member,another_member' self.team1.skip_team_pr = True self.dev_pr.pr_author = 'some_member' - self.config_step._run_codeowner(self.parent_build, '/tmp/essai') + self.config_step._run_codeowner(self.parent_build) messages = self.parent_build.log_ids.mapped('message') self.assertEqual(messages[5], "Skipping teams ['team_py'] since author is part of the team members") self.assertEqual(messages[6], 'Requesting review for pull request [base/server:1234](https://example.com/base/server/pull/1234): codeowner-team, team_js') @@ -155,7 +155,7 @@ class TestCodeowner(TestBuildConfigStepCommon): self.diff = '\n'.join([ 'core/addons/module1/some/file.py', ]) - self.config_step._run_codeowner(self.parent_build, '/tmp/essai') + self.config_step._run_codeowner(self.parent_build) messages = self.parent_build.log_ids.mapped('message') self.assertEqual( messages[2], @@ -168,7 +168,7 @@ class TestCodeowner(TestBuildConfigStepCommon): self.diff = '\n'.join([ 'core/addons/module1/some/file.py', ]) - self.config_step._run_codeowner(self.parent_build, '/tmp/essai') + self.config_step._run_codeowner(self.parent_build) messages = self.parent_build.log_ids.mapped('message') self.assertEqual( messages[2], @@ -186,7 +186,7 @@ class TestCodeowner(TestBuildConfigStepCommon): 'core/addons/module3/some/file.js', 'core/addons/module4/some/file.txt', ]) - self.config_step._run_codeowner(self.parent_build, '/tmp/essai') + self.config_step._run_codeowner(self.parent_build) messages = self.parent_build.log_ids.mapped('message') self.assertEqual(messages, [ 'PR [base/server:1234](https://example.com/base/server/pull/1234) found for repo **server**', @@ -215,7 +215,7 @@ class TestBuildConfigStepCreate(TestBuildConfigStepCommon): """ Test child builds are taken into account""" - self.config_step._run_create_build(self.parent_build, '/tmp/essai') + self.config_step._run_create_build(self.parent_build) self.assertEqual(len(self.parent_build.children_ids), 2, 'Two sub-builds should have been generated') # check that the result will be ignored by parent build @@ -229,7 +229,7 @@ class TestBuildConfigStepCreate(TestBuildConfigStepCommon): def test_config_step_create(self): """ Test the config step of type create """ self.config_step.make_orphan = True - self.config_step._run_create_build(self.parent_build, '/tmp/essai') + self.config_step._run_create_build(self.parent_build) self.assertEqual(len(self.parent_build.children_ids), 2, 'Two sub-builds should have been generated') # check that the result will be ignored by parent build @@ -252,7 +252,7 @@ class TestBuildConfigStepCreate(TestBuildConfigStepCommon): }).id, }) - self.config_step._run_create_build(self.parent_build, '/tmp/essai') + self.config_step._run_create_build(self.parent_build) self.assertEqual(len(self.parent_build.children_ids), 10, '10 build should have been generated') # check that the result will be ignored by parent build @@ -271,7 +271,7 @@ class TestBuildConfigStepCreate(TestBuildConfigStepCommon): }).id, }) - self.config_step._run_create_build(self.parent_build, '/tmp/essai') + self.config_step._run_create_build(self.parent_build) self.assertEqual(len(self.parent_build.children_ids), 5, '5 build should have been generated') # check that the result will be ignored by parent build @@ -295,7 +295,7 @@ class TestBuildConfigStepCreate(TestBuildConfigStepCommon): }).id, }) - self.config_step._run_create_build(self.parent_build, '/tmp/essai') + self.config_step._run_create_build(self.parent_build) self.assertEqual(len(self.parent_build.children_ids), 10, '10 build should have been generated') self.assertEqual(len(self.parent_build.children_ids.filtered(lambda b: b.config_id == test_config_1)), 5) self.assertEqual(len(self.parent_build.children_ids.filtered(lambda b: b.config_id == test_config_2)), 5) @@ -382,7 +382,7 @@ class TestBuildConfigStep(TestBuildConfigStepCommon): self.assertEqual(log_path, 'dev/null/logpath') self.patchers['docker_run'].side_effect = docker_run - config_step._run_install_odoo(self.parent_build, 'dev/null/logpath') + config_step._run_install_odoo(self.parent_build) @patch('odoo.addons.runbot.models.build.BuildResult._checkout') def test_dump(self, mock_checkout): @@ -401,7 +401,7 @@ class TestBuildConfigStep(TestBuildConfigStepCommon): self.patchers['docker_run'].side_effect = docker_run - config_step._run_install_odoo(self.parent_build, 'dev/null/logpath') + config_step._run_install_odoo(self.parent_build) @patch('odoo.addons.runbot.models.build.BuildResult._checkout') def test_install_tags(self, mock_checkout): @@ -424,7 +424,7 @@ class TestBuildConfigStep(TestBuildConfigStepCommon): self.assertEqual(tags, '/module,:class.method') self.patchers['docker_run'].side_effect = docker_run - config_step._run_install_odoo(self.parent_build, 'dev/null/logpath') + config_step._run_install_odoo(self.parent_build) config_step.enable_auto_tags = True @@ -435,7 +435,7 @@ class TestBuildConfigStep(TestBuildConfigStepCommon): self.assertEqual(tags, '/module,:class.method,-:otherclass.othertest') self.patchers['docker_run'].side_effect = docker_run2 - config_step._run_install_odoo(self.parent_build, 'dev/null/logpath') + config_step._run_install_odoo(self.parent_build) @patch('odoo.addons.runbot.models.build.BuildResult._checkout') def test_db_name(self, mock_checkout): @@ -455,19 +455,19 @@ class TestBuildConfigStep(TestBuildConfigStepCommon): self.patchers['docker_run'].side_effect = docker_run - config_step._run_step(self.parent_build, 'dev/null/logpath') + config_step._run_step(self.parent_build) assert_db_name = 'custom_build' parent_build_params = self.parent_build.params_id.copy({'config_data': {'db_name': 'custom_build'}}) parent_build = self.parent_build.copy({'params_id': parent_build_params.id}) - config_step._run_step(parent_build, 'dev/null/logpath') + config_step._run_step(parent_build) config_step = self.ConfigStep.create({ 'name': 'run_test', 'job_type': 'run_odoo', 'custom_db_name': 'custom', }) - config_step._run_step(parent_build, 'dev/null/logpath') + config_step._run_step(parent_build) self.assertEqual(call_count, 3) @@ -489,7 +489,7 @@ docker_params = dict(cmd=cmd) self.assertIn('-d test_database', run_cmd) self.patchers['docker_run'].side_effect = docker_run - config_step._run_step(self.parent_build, 'dev/null/logpath') + config_step._run_step(self.parent_build) self.patchers['docker_run'].assert_called_once() db = self.env['runbot.database'].search([('name', '=', 'test_database')]) self.assertEqual(db.build_id, self.parent_build) @@ -506,7 +506,7 @@ def run(): 'python_code': test_code, }) - retult = config_step._run_python(self.parent_build, 'dev/null/logpath') + retult = config_step._run_python(self.parent_build) self.assertEqual(retult, {'a': 'b'}) @patch('odoo.addons.runbot.models.build.BuildResult._checkout') @@ -525,7 +525,7 @@ def run(): call_count += 1 self.patchers['docker_run'].side_effect = docker_run - config_step._run_step(self.parent_build, 'dev/null/logpath') + config_step._run_step(self.parent_build) self.assertEqual(call_count, 1) diff --git a/runbot_cla/build_config.py b/runbot_cla/build_config.py index 6cd6800e..d4c461eb 100644 --- a/runbot_cla/build_config.py +++ b/runbot_cla/build_config.py @@ -15,7 +15,7 @@ class Step(models.Model): job_type = fields.Selection(selection_add=[('cla_check', 'Check cla')], ondelete={'cla_check': 'cascade'}) - def _run_cla_check(self, build, log_path): + def _run_cla_check(self, build): build._checkout() cla_glob = glob.glob(build._get_server_commit()._source_path("doc/cla/*/*.md")) error = False