7
\$\begingroup\$

I've written a Python script that given a few arguments executes a build process bash script.

def start(self):
        """

        """
        repository = Repository.query \
            .join(Client) \
            .filter(Client.id == self.client_id) \
            .first()

        client = Client.query.filter(Client.id == self.client_id).first()

        record = Build(
            client_id=self.client_id,
            branch=self.branch,
            repository_id=repository.id,
            state=self.State.PENDING.value,
            started_at=datetime.datetime.now())
        self._session.add(record)
        self._session.commit()

        wd = os.path.join(BuildServerConfig.output_dir, str(record.id))

        try:
            return_code = subprocess.call(args=[
                '/bin/bash', BuildServerConfig.build_sh,
                '--repository_url={}'.format(repository.url),
                '--branch={}'.format(self.branch),
                '--build_id={}'.format(record.id),
                '--client_id={}'.format(self.client_id),
                '--signing={}'.format(self.signing),
                '--target={}'.format(self.target)
            ])
        except Exception as exception:
            logging.error(exception)
        finally:
            self.finalize(return_code)

            final_state = None
            if return_code != 0:
                # Send notification email with logs
                # for diagnosis attached. The recipient
                # of the email is the last author that
                # made the commit. It is retrieving by
                # regex matching the git last log entry.
                # The email is send only if the recipient's
                # email address has been found.

                git_dir = os.path.join(wd, 'source')
                if os.path.exists(git_dir):
                    log = git.Git(git_dir).log('-1')
                    if log is not None:
                        pattern = r'\b[a-zA-Z0-9.-]+@[a-zA-Z0-9.-]+\.[a-zA-Z0-9.-]+\b'
                        match = re.search(pattern, log)
                        if match is not None:
                            message = emails.Message(
                                subject='{0} build for branch {1} failed'.format(client.name, self.branch),
                                mail_from=('Build server', 'buildserver.<domain>'),
                                text='The build has failed. Please diagnose the problem using the attached log file.')

                            build_log = os.path.join(wd, 'build.log')
                            if os.path.exists(build_log) and os.path.isfile(build_log):
                                message.attach(data=open(log, 'rb'), filename='build.log')

                                response = message.send(to=match.group(1),
                                        smtp=BuildServerConfig.smtp)
                                if response != 250:
                                    logging.error(response.error)

                final_state = self.State.FAILED
            else:
                final_state = self.State.FINISHED

            # Send push notifications to all registered
            # devices.
            # TODO: author should be the only recipient.

            notification = apn.notification(
                alert='Build for {0}\'s branch {1} {2}'
                    .format(client.name, self.branch, final_state.value),
                sound='default')

            tokens = [device.token for device in UserDevice.query.all()]
            notification.send(
                tokens,
                cert_file=BuildServerConfig.apn_cert_file,
                key_file=BuildServerConfig.apn_key_file)

            # Update state on completion of the
            # build execution.
            record.state = final_state.value
            record.finished_at = datetime.datetime.now()
            self._session.commit()
            self._session.close()

Upon completion, the script is expected to e.g. send email notifications with logs attached in the case of failure, send push notifications etcetera.

However, the method is quite monolithic and decreasing testability. On the other hand, I am not expecting to reuse any of the parts of the function. Hence, my question is, how would one refactor this code fragment to make it more Pythonic and readable as well, while keeping in mind principles such as needless complexity?

\$\endgroup\$
  • \$\begingroup\$ I'm using Python 3.6 \$\endgroup\$ – dsafa Feb 7 '17 at 13:16
3
\$\begingroup\$

On the whole, it looks good.

I feel the finally clause is too long. Please break out that code into a helper function. (Or, put another way, start() is too long, and that clause appears to be low-hanging fruit.)

I wonder if it would be helpful for a with clause to accomplish these cleanup actions:

        self._session.commit()
        self._session.close()

You mentioned certain frustrations with testability, for example it might be slightly challenging to arrange for return_code to be non-zero. Breaking out helper functions would make that moot, as you could just directly call such code without needing to make return_code take on a particular value.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.