Merge ~lofidevops/ols-jenkaas:sn1780-refactor into ols-jenkaas:main

[David]

Refactor land_proposal

No functional changes. SonarLint complained about the code complexity and I agree.

The major changes are decanting the logic of _run_landing_tests_on_host() and _run_landing_tests_in_container()

[Maximiliano Bertacchini (maxiberta)]

Nice refactor!! Thanks

review: Approve

[David (lofidevops)]

Guillo confirmed our target (Python 3.6 on Bionic) which supports f-strings.

diff --git a/olsjenkaas/commands.py b/olsjenkaas/commands.py
index a6be897..16c689b 100644
--- a/olsjenkaas/commands.py
+++ b/olsjenkaas/commands.py
@@ -483,81 +483,103 @@ class LandApprovedProposal(CommandWithContainer):
         proposal.setStatus(status='Needs review',
                            revid=proposal.reviewed_revid)
 
+    def _run_landing_tests_on_host(self, proposal, local_work_dir):
+
+        logger.info(f"Running {self.options.setup_cmd}")
+        if self.run_in_host(
+                ['cd', local_work_dir, '&&', self.options.setup_cmd]):
+            logger.error(f"{self.options.setup_cmd} failed.")
+            raise Rejected("Project setup failed.")
+
+        # commit diff right before testing so we can query for
+        # the right revno, and still have setup command make
+        # changes
+        self.commit_proposal(local_work_dir, proposal)
+
+        logger.info(f"Running {self.options.test_cmd}")
+        if self.run_in_host(
+                ["cd", local_work_dir, "&&", self.options.test_cmd]):
+            logger.error(f"{self.options.test_cmd} failed")
+            raise Rejected("Running landing tests failed.")
+
+    def _run_landing_tests_in_container(self, proposal, local_work_dir):
+        mounts, remote_work_dir = self.get_mounts_and_remote(local_work_dir)
+
+        logger.info("Creating worker...")
+        with olsjenkaas.working_directory(local_work_dir):
+            self.create_worker(self.options.container_name,
+                               mounts=mounts)
+        logger.info(f"{self.worker.name} setup on top of {self.worker.backing_name}")
+
+        logger.info(f"Running {self.options.setup_cmd}")
+        if self.run_in_container(
+                ["cd", remote_work_dir, "&&", self.options.setup_cmd],
+                with_creds=True):
+            logger.error(f"{self.options.setup_cmd} failed.")
+            raise Rejected("Project setup failed.")
+
+        # commit diff right before testing so we can query for
+        # the right revno, and still have setup command make
+        # changes
+        self.commit_proposal(local_work_dir, proposal)
+
+        logger.info(f"Running {self.options.test_cmd}")
+        if self.run_in_container(
+                ["cd", remote_work_dir, "&&", self.options.test_cmd]):
+            logger.error(f"{self.options.test_cmd} failed.")
+            raise Rejected("Running landing tests failed.")
+
     def land_proposal(self, proposal, target_url):
+        """
+        Land a proposal (validate, merge, run landing tests, push upstream).
+        Returns 0 if successful, otherwise 1.
+        """
 
-        logger.info('Trying to land {}'.format(proposal.web_link))
+        logger.info(f"Trying to land {proposal.web_link}")
         try:
+
+            # validate proposal
             if not proposal.commit_message:
-                raise Rejected('A commit message must be set')
+                raise Rejected("A commit message must be set.")
             if not launchpad.is_approved(proposal, self.target):
-                raise Rejected('Voting criteria not met')
+                raise Rejected("Voting criteria not met.")
+
+            # prepare merge target
             local_work_dir = os.path.realpath(self.options.work_dir)
-            logger.info('Getting {}'.format(target_url))
+            logger.info(f"Getting {target_url}")
             self.setup_merge_target(local_work_dir, proposal, target_url)
-            logger.info('Merging {}'.format(proposal.web_link))
-            logger.info(
-                'Approved revision is {}'.format(proposal.reviewed_revid))
+            logger.info(f"Merging {proposal.web_link}")
+            logger.info(f"Approved revision is {proposal.reviewed_revid}")
+
+            # merge
             try:
                 merged = self.merge_proposed_branch(local_work_dir, proposal)
             except errors.CommandError:
-                raise Rejected('Merging failed')
+                raise Rejected("Merging failed.")
             if not merged:
-                logger.error('Nothing to merge from {}'.format(
-                    proposal.web_link))
-                raise Rejected('There was nothing to merge')
-            mounts, remote_work_dir = self.get_mounts_and_remote(
-                local_work_dir)
-            if self.options.container_name == '':
+                logger.error(f"Nothing to merge from {proposal.web_link}")
+                raise Rejected("There was nothing to merge.")
+
+            # run landing tests in appropriate environment
+            if self.options.container_name == "":
                 # no container name specified, run in host
-                logger.info('Running {}'.format(self.options.setup_cmd))
-                if self.run_in_host(
-                       ['cd', local_work_dir, '&&', self.options.setup_cmd]):
-                    logger.error('{} failed'.format(self.options.setup_cmd))
-                    raise Rejected('Project setup failed')
-                # commit diff right before testing so we can query for
-                # the right revno, and still have setup command make
-                # changes
-                self.commit_proposal(local_work_dir, proposal)
-                logger.info('Running {}'.format(self.options.test_cmd))
-                if self.run_in_host(
-                        ['cd', local_work_dir, '&&', self.options.test_cmd]):
-                    logger.error('{} failed'.format(self.options.test_cmd))
-                    raise Rejected('Running landing tests failed')
+                self._run_landing_tests_on_host(proposal, local_work_dir)
             else:
                 # run in named container
-                logger.info('Creating worker...')
-                with olsjenkaas.working_directory(local_work_dir):
-                    self.create_worker(self.options.container_name,
-                                       mounts=mounts)
-                logger.info('{} setup on top of {}'.format(
-                    self.worker.name, self.worker.backing_name))
-                logger.info('Running {}'.format(self.options.setup_cmd))
-                if self.run_in_container(
-                        ['cd', remote_work_dir, '&&', self.options.setup_cmd],
-                        with_creds=True):
-                    logger.error('{} failed'.format(self.options.setup_cmd))
-                    raise Rejected('Project setup failed')
-                # commit diff right before testing so we can query for
-                # the right revno, and still have setup command make
-                # changes
-                self.commit_proposal(local_work_dir, proposal)
-                logger.info('Running {}'.format(self.options.test_cmd))
-                if self.run_in_container(
-                        ['cd', remote_work_dir, '&&', self.options.test_cmd]):
-                    logger.error('{} failed'.format(self.options.test_cmd))
-                    raise Rejected('Running landing tests failed')
-            # merge was already committed, and landing tests passed, so the
-            # only thing left to do is to push the branch upstream
+                self._run_landing_tests_in_container(proposal, local_work_dir)
+
+            # push merged branch upstream
             self.push_merged_proposal(local_work_dir, proposal, target_url)
             # FIXME: Once webhooks are used, lp can be trusted to do that (with
             # some delay), revert this once this has been verified
             # -- vila 2017-03-30
-            proposal.setStatus(status='Merged')
-            logger.info('{} has landed'.format(proposal.web_link))
+
+            proposal.setStatus(status="Merged.")
+            logger.info(f"{proposal.web_link} has landed.")
             return 0
+
         except Rejected as e:
-            logger.info('{} could not be landed: {}'.format(
-                proposal.web_link, e.reason))
+            logger.info(f"{proposal.web_link} could not be landed: {e.reason}")
             self.reject_proposal(proposal, e.reason)
             return 1
         finally: