launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/buildmaster/model/buildqueue.py'
> --- lib/lp/buildmaster/model/buildqueue.py	2016-05-14 00:25:07 +0000
> +++ lib/lp/buildmaster/model/buildqueue.py	2017-04-18 13:27:48 +0000
> @@ -180,6 +180,21 @@
>          if builder is not None:
>              del get_property_cache(builder).currentjob
>  
> +    def collectStatus(self, slave_status):
> +        """See `IBuildQueue`."""
> +        builder_status = slave_status["builder_status"]
> +        if builder_status == "BuilderStatus.ABORTING":
> +            self.logtail = "Waiting for slave process to be terminated"
> +        elif slave_status.get("logtail") is not None:
> +            # slave_status["logtail"] is normally an xmlrpclib.Binary
> +            # instance, and the contents might include invalid UTF-8 due to
> +            # being a fixed number of bytes from the tail of the log.  Turn
> +            # it into Unicode as best we can.
> +            self.logtail = str(
> +                slave_status.get("logtail")).decode("UTF-8", errors="replace")
> +        self.specific_build.updateStatus(
> +            self.specific_build.status, slave_status=slave_status)

I vaguely dislike the hidden updateStatus call, though it's not dreadful. More problematic is the fact that we could totally miss the revision ID if we don't scan at the right time, though maybe there's no easy way to avoid that.

> +
>      def suspend(self):
>          """See `IBuildQueue`."""
>          if self.status != BuildQueueStatus.WAITING:
> 
> === modified file 'lib/lp/snappy/templates/snapbuild-index.pt'
> --- lib/lp/snappy/templates/snapbuild-index.pt	2017-02-27 18:46:38 +0000
> +++ lib/lp/snappy/templates/snapbuild-index.pt	2017-04-18 13:27:48 +0000
> @@ -112,6 +112,9 @@
>      </p>
>  
>      <ul>
> +      <li id="revision-id" tal:condition="context/revision_id">
> +        Revision: <span tal:replace="context/revision_id" />
> +      </li>

This isn't actually going to be the Bazaar revision ID, is it?

>        <li tal:condition="context/dependencies">
>          Missing build dependencies: <em tal:content="context/dependencies"/>
>       </li>


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-revision-id/+merge/321690
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.