launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

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 could move the updateStatus up to the caller if you'd prefer?  There are a few other places where we poke at the behaviour directly from the interactor.  It seemed like reasonable encapsulation, though.

The revision ID is returned by any builder status call after the repo phase has finished, so as far as I can see it can only be missed if the builder fails catastrophically.  I'm OK with 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>

At the moment https://code.launchpad.net/~cjwatson/launchpad-buildd/extended-snap-status/+merge/321689 returns the revno in the Bazaar case.  Do you think it should be the revision ID instead?  I'm undecided, since the revno is potentially ambiguous but seems more likely to be what Bazaar users expect.  We could adjust things a bit further to save both revno and revision ID in the Bazaar case ...

>        <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.