launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'configs/development/launchpad-lazr.conf'
> --- configs/development/launchpad-lazr.conf	2015-02-19 17:33:35 +0000
> +++ configs/development/launchpad-lazr.conf	2016-02-01 21:05:05 +0000
> @@ -96,7 +96,7 @@
>  enable_test_openid_provider: True
>  openid_provider_root: https://testopenid.dev/
>  code_domain: code.launchpad.dev
> -default_batch_size: 5
> +default_batch_size: 50

Please revert this.  The local batch size is intentionally small to catch problems with poor batching in the test suite and to make it easier to reproduce test suite problems locally; it's fine to edit it locally for convenience if you need to (I've done that too from time to time), but don't commit it.

>  max_attachment_size: 2097152
>  branchlisting_batch_size: 6
>  mugshot_batch_size: 8
> @@ -184,6 +184,13 @@
>  password: guest
>  virtual_host: /
>  
> +[snappy]
> +builder_proxy_auth_api_admin_username: admin-launchpad.net

It would be less confusing if this could be admin-launchpad.dev (and likewise admin-qastaging.launchpad.net on qastaging), though I realise that will involve spec changes to set admin_api_username.  Ideally the charm would default to launchpad.dev, not launchpad.net.

> +builder_proxy_auth_api_endpoint: http://snap-proxy.launchpad.dev:8080/tokens
> +builder_proxy_host: snap-proxy.launchpad.dev
> +builder_proxy_port: 3128
> +tools_source: deb http://ppa.launchpad.net/snappy-dev/snapcraft-daily/ubuntu %(series)s main
> +
>  [txlongpoll]
>  launch: True
>  frontend_port: 22435
> 
> === modified file 'lib/lp/buildmaster/model/buildfarmjobbehaviour.py'
> --- lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2015-02-17 09:33:33 +0000
> +++ lib/lp/buildmaster/model/buildfarmjobbehaviour.py	2016-02-01 21:05:05 +0000
> @@ -77,7 +77,7 @@
>              "Preparing job %s (%s) on %s."
>              % (cookie, self.build.title, self._builder.url))
>  
> -        builder_type, das, files, args = self.composeBuildRequest(logger)
> +        builder_type, das, files, args = yield self.composeBuildRequest(logger)

I'm suspicious that this branch doesn't involve any changes to derived classes of BuildFarmJobBehaviour other than SnapBuildBehaviour.  Have you run the tests for those other behaviours (-t buildbehaviour -t recipebuilder should catch them all)?  At least, I think it would be less confusing to make all their composeBuildRequest implementations explicitly be generators, even if they end up being trivial cases of generators.

>  
>          # First cache the chroot and any other files that the job needs.
>          chroot = das.getChroot()
> 
> === modified file 'lib/lp/snappy/model/snapbuildbehaviour.py'
> --- lib/lp/snappy/model/snapbuildbehaviour.py	2015-09-10 17:18:00 +0000
> +++ lib/lp/snappy/model/snapbuildbehaviour.py	2016-02-01 21:05:05 +0000
> @@ -96,12 +115,36 @@
>              raise CannotBuild(
>                  "Source branch/repository for ~%s/%s has been deleted." %
>                  (build.snap.owner.name, build.snap.name))
> -        return args
> -
> +        defer.returnValue(args)
> +
> +    @defer.inlineCallbacks
> +    def _requestProxyToken(self):
> +        admin_username = config.snappy.builder_proxy_auth_api_admin_username
> +        secret = config.snappy.builder_proxy_auth_api_admin_secret
> +        url = config.snappy.builder_proxy_auth_api_endpoint
> +        timestamp = (datetime.utcnow() - datetime(1970, 1, 1)).total_seconds()

I *think* that amounts to a complicated spelling of int(time.time()).

> +        proxy_username = '{build_id}-{timestamp}'.format(
> +            build_id=self.build.build_cookie,
> +            timestamp=timestamp)
> +        auth_string = '{}:{}'.format(admin_username, secret).strip()
> +        auth_header = 'Basic ' + base64.encodestring(auth_string)
> +        data = json.dumps({'username': proxy_username})
> +
> +        result = yield getPage(
> +            url,
> +            method='POST',
> +            postdata=data,
> +            headers={
> +                'Authorization': auth_header,
> +                'Content-Type': 'application/json'}
> +            )
> +        token = json.loads(result)
> +        defer.returnValue(token)
> +
> +    @defer.inlineCallbacks
>      def composeBuildRequest(self, logger):
> -        return (
> -            "snap", self.build.distro_arch_series, {},
> -            self._extraBuildArgs(logger=logger))
> +        args = yield self._extraBuildArgs(logger=logger)
> +        defer.returnValue(("snap", self.build.distro_arch_series, {}, args))
>  
>      def verifySuccessfulBuild(self):
>          """See `IBuildFarmJobBehaviour`."""
> 
> === modified file 'lib/lp/snappy/tests/test_snapbuildbehaviour.py'
> --- lib/lp/snappy/tests/test_snapbuildbehaviour.py	2015-09-16 19:07:43 +0000
> +++ lib/lp/snappy/tests/test_snapbuildbehaviour.py	2016-02-01 21:05:05 +0000
> @@ -5,8 +5,18 @@
>  
>  __metaclass__ = type
>  
> +from datetime import datetime
> +from mock import (
> +    patch,
> +    Mock,
> +    )

mock isn't in the stdlib in Python 2, so this belongs in the next imports section down.  (utilities/format-new-and-modified-imports)

> +
>  import fixtures
>  import transaction
> +from testtools import ExpectedException
> +from testtools.deferredruntest import AsynchronousDeferredRunTest
> +from testtools.matchers import IsInstance
> +from twisted.internet import defer
>  from twisted.trial.unittest import TestCase as TrialTestCase
>  from zope.component import getUtility
>  
> @@ -154,6 +168,42 @@
>          e = self.assertRaises(CannotBuild, job.verifyBuildRequest, logger)
>          self.assertIn("Missing chroot", str(e))
>  
> +
> +class TestAsyncSnapBuildBehaviour(TestSnapBuildBehaviourBase):
> +    run_tests_with = AsynchronousDeferredRunTest
> +
> +    def setUp(self):
> +        super(TestAsyncSnapBuildBehaviour, self).setUp()
> +        self.token = {'secret': '03368addc7994647ace69e7ac2eb1ae9',

Maybe a comment to say that this is an arbitrary UUID, if you don't want to generate a different one for each test.

> +                      'username': 'SNAPBUILD-1',
> +                      'timestamp': datetime.utcnow().isoformat()}
> +        self.proxy_url = ("http://{username}:{password}";
> +                          "@{host}:{port}".format(
> +                              username=self.token['username'],
> +                              password=self.token['secret'],
> +                              host=config.snappy.builder_proxy_host,
> +                              port=config.snappy.builder_proxy_port))
> +        self.patcher = patch.object(
> +            SnapBuildBehaviour, '_requestProxyToken',
> +            Mock(return_value=self.mockRequestProxyToken())).start()
> +
> +    def tearDown(self):
> +        super(TestAsyncSnapBuildBehaviour, self).tearDown()
> +        self.patcher.stop()
> +
> +    def mockRequestProxyToken(self):
> +        return defer.succeed(self.token)
> +
> +    @defer.inlineCallbacks
> +    def test_composeBuildRequest(self):
> +        job = self.makeJob()
> +        lfa = self.factory.makeLibraryFileAlias(db_only=True)
> +        job.build.distro_arch_series.addOrUpdateChroot(lfa)
> +        build_request = yield job.composeBuildRequest(None)
> +        self.assertEqual(build_request[1], job.build.distro_arch_series)
> +        self.assertThat(build_request[3], IsInstance(dict))
> +
> +    @defer.inlineCallbacks
>      def test_extraBuildArgs_bzr(self):
>          # _extraBuildArgs returns appropriate arguments if asked to build a
>          # job for a Bazaar branch.


-- 
https://code.launchpad.net/~blr/launchpad/build-manager-proxy-client/+merge/283372
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.