Merge lp:~jtv/launchpad/bug-536797 into lp:launchpad
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | not available | ||||
Proposed branch: | lp:~jtv/launchpad/bug-536797 | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
814 lines (+356/-89) 23 files modified
lib/canonical/launchpad/interfaces/_schema_circular_imports.py (+2/-1) lib/canonical/launchpad/security.py (+52/-2) lib/canonical/launchpad/webapp/configure.zcml (+7/-0) lib/canonical/launchpad/webapp/tales.py (+27/-0) lib/lp/buildmaster/interfaces/buildfarmbranchjob.py (+21/-0) lib/lp/buildmaster/interfaces/buildfarmjob.py (+0/-1) lib/lp/buildmaster/interfaces/buildqueue.py (+4/-0) lib/lp/buildmaster/model/buildqueue.py (+12/-2) lib/lp/buildmaster/tests/test_buildqueue.py (+36/-7) lib/lp/code/interfaces/sourcepackagerecipebuild.py (+2/-7) lib/lp/soyuz/browser/build.py (+0/-1) lib/lp/soyuz/browser/builder.py (+2/-8) lib/lp/soyuz/browser/configure.zcml (+31/-1) lib/lp/soyuz/doc/build-estimated-dispatch-time.txt (+3/-4) lib/lp/soyuz/interfaces/buildfarmbuildjob.py (+21/-0) lib/lp/soyuz/interfaces/buildpackagejob.py (+4/-4) lib/lp/soyuz/templates/builder-index.pt (+5/-47) lib/lp/soyuz/templates/buildfarmbranchjob-current.pt (+11/-0) lib/lp/soyuz/templates/buildfarmbuildjob-current.pt (+8/-0) lib/lp/soyuz/templates/buildfarmjob-current.pt (+10/-0) lib/lp/soyuz/templates/buildqueue-current.pt (+24/-0) lib/lp/translations/model/translationtemplatesbuildjob.py (+5/-4) lib/lp/translations/stories/buildfarm/xx-build-summary.txt (+69/-0) |
||||
To merge this branch: | bzr merge lp:~jtv/launchpad/bug-536797 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Brad Crittenden (community) | code | Approve | |
Canonical Launchpad Engineering | ui | Pending | |
Review via email: mp+21439@code.launchpad.net |
Commit message
Minimal support for non-Soyuz build-farm jobs in builder UI.
Description of the change
= Bug 536797 =
The Builder UI is not very good at dealing with non-Soyuz build-farm jobs. Most build-farm job types have an IBuildBase associated with them, but not all. (It's not required by the IBuildFarmJob interface either).
This branch, after discussion with al-maisan, bigjools, noodles, wgrant, and probably others introduces a bit more UI support for coping with different types of build-farm job. It does this by introducing a few new interfaces: IBuildFarmBuildJob represents an IBuildFarmJob that also has an IBuild reference, and IBuildFarmBranchJob represents one that has an IBranch reference. These inherit from IBuildFarmJob so that zope can pick the most specific applicable interface to render in the UI.
You'll see a bunch of new templates: buildqueue-
I also added a link formatter for IBuildBase, to help cut down on complicated logic in the TAL.
In the same vein you'll see the access check for showing a job's log tail moved into security.py. Doing this in the TAL became too complicated with the different build types. It's quite possible that the current check is too strict; it may make sense to show a build's log if either its branch or its build is accessible. I think experience will have to show this. Soyuz is not the kind of thing you perfect all in one branch. In any case it's hard to exercise just now: our jobs that generate translation templates don't support private branches.
A BuildQueue's "current build duration" moves from the view into the model, which seems more appropriate and lets it reuse a "fake clock" fixture that's already there. I did replace some use of datetime.utcnow() with datetime.
You'll note that some of the files I introduced deserve to be in better places. I filed a separate bug about this, because I've just about hit the limit of what I can do in one branch, and a step like that would need broader discussion.
To test (runs a lot of tests, but many exercise one change or another):
{{{
./bin/test -vv -t buildmaster -t soyuz -t 'translations.
}}}
To Q/A, first off, browser around the /builders page. Then follow the instructions on https:/
I found no lint. I'm firing off an EC2 run to make sure I didn't miss any tests.
Jeroen
Hi Jeroen,
Thanks for the nice agrarian branch.
Running your test command (was that just 'make check' in disguise?) I ildToSlave.
got a failure on test_dispatchBu
Jeroen I tried on two different occasions to follow your instructions
in order to exercise the new UI. Both times were very time consuming
and ultimately failed. I give up. I approve the code but not the UI
-- you needed to get a separate UI review anyway but as a code
reviewer I like to have a once-over.
> === modified file 'lib/canonical/ launchpad/ security. py' launchpad/ security. py 2010-03-12 08:59:36 +0000 launchpad/ security. py 2010-03-16 11:13:21 +0000 checkUnauthenti cated() b(Authorization Base): sion(self) : (self.obj. build) ibility( self, user=None): hJob.providedBy (self.obj) : branch. visibleByUser( user)
> --- lib/canonical/
> +++ lib/canonical/
> @@ -1505,6 +1508,41 @@
> return auth_spr.
>
>
> +class ViewBuildFarmJo
> + """Permission to view an `IBuildFarmJob`.
> +
> + This permission is based entirely on permission to view the
> + associated `IBuild` and/or `IBranch`.
> + """
> + permission = 'launchpad.View'
> + usedfor = IBuildFarmJob
> +
> + def _getBuildPermis
> + return ViewBuildRecord
> +
> + def _checkBranchVis
> + """Is the user free to view any branches associated with this job?"""
> + if not IBuildFarmBranc
> + return True
> + else:
> + return self.obj.
I don't like the inverted test (our coding standards recommend against
having a 'not' test if both paths are present) and the fact a return
value of True has dual meanings.
> + def checkAuthentica ted(self, user): chVisibility( user): Job.providedBy( self.obj) : ermission( ).checkAuthenti cated(user) :
> + if not self._checkBran
> + return False
> +
> + if IBuildFarmBuild
> + if not self._getBuildP
> + return False
> +
> + return True
Could you write a single method that provides checkAuthenticated and cated for your two possiblities? A nice little wrapper
checkUnauthenti
would make the real methods more readable.
> + def checkUnauthenti cated(self) : chVisibility( ): ermission( ).checkUnauthen ticated( )
> + if not self._checkBran
> + return False
> + return self._getBuildP
Again I find this implementation confusing.
> + AdminByAdminsTe am):
> class AdminQuestion(
> permission = 'launchpad.Admin'
> usedfor = IQuestion
> === modified file 'lib/canonical/ launchpad/ webapp/ tales.py' launchpad/ webapp/ tales.py 2010-03-10 12:50:18 +0000 launchpad/ webapp/ tales.py 2010-03-16 11:13:21 +0000 terAPI( ObjectFormatter API):
> --- lib/canonical/
> +++ lib/canonical/
> @@ -1514,6 +1514,33 @@
> return url
>
>
> +class BuildBaseFormat
> + """Adapter providing fmt support for `IBuildBase` objects."""
You have plenty of room so please spell out formatter.
> + def _composeArchive Reference( self, archive): archive. owner.name) , cgi.escape( archive. name))
> + if archive.is_ppa:
> + return " [%s/%s]" % (
> + cgi.escape(
> + else:
> + return ""
> +
> + def icon(self, view_name):
> + if not check_permi...