[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

@@ -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)

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

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')

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)
 

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