Merge lp:~robru/bileto/publish-job into lp:bileto

[Robert Bruce Park]

Implement publish job in Bileto, obsoleting jenkins entirely.

(note this depends on charm changes and must be deployed with "mojo upgrade-charm")

[lp:~robru/bileto/publish-job]

703. By Robert Bruce Park on 2016-08-18

Temporarily disable some checks for testing.

704. By Robert Bruce Park on 2016-08-19

Temporarily disable some more checks for testing.

705. By Robert Bruce Park on 2016-08-19

Temporarily disable even more checks for testing.

706. By Robert Bruce Park on 2016-08-19

Fix publishing.

707. By Robert Bruce Park on 2016-08-19

Re-enable all checks.

[Martin Pitt (pitti)]

Mostly code style issues and nitpicks, but two or three more serious issues.

review: Needs Fixing

[Robert Bruce Park (robru)]

Replies inline,thanks Martin

[Martin Pitt (pitti)]

Answered to your replies, thanks!

The flashplugin-nonfree component issue is still a blocker. The rsync race looks like an actual bug, but not that important (so not a blocker). Do with the code style/obfuscation comments what you want.

[lp:~robru/bileto/publish-job]

708. By Robert Bruce Park on 2016-08-22

Rename __getattr__ to call_on_all_packages for pitti.

709. By Robert Bruce Park on 2016-08-22

Ensure os.rename() stays within filesystem.

710. By Robert Bruce Park on 2016-08-22

Clarify docstring.

[Robert Bruce Park (robru)]

Ok I've pushed commits to resolve the issues I agree with, I'm choosing not to block on the two things that are historical cruft that I've inherited (flashplugin-nonfree/universe and '0 means no version'), I'm open to revisiting those later but for now those things work and are already production-proven, and I need to move forward with this rollout. Thanks for the review Martin!

review: Approve

[lp:~robru/bileto/publish-job]

711. By Robert Bruce Park on 2016-08-22

Fix tests.

[Martin Pitt (pitti)]

Robert Bruce Park [2016-08-22 16:06 -0000]:
> Can you elaborate what you mean by "recursively call the method on its direct children"?

Well, I don't know what this is doing, but you said that __getattr__()
has some magic to do that kind of iteration.

> The reason that __getattr__ is defined to pass methods down to
> children is that this is called from dozens of places (and many of
> those cases it isn't overloaded at all, it's just a direct pass
> through), so I'm not going to reimplement that in every single place
> that needs to pass through. I'll probably rename __getattr__() to
> call_on_all_packages().

Doing "__getattr__ = call_on_all_packages" and replacing half of the
calls with the other makes things even *more*
confusing/implicit/surprising, so better revert that bit or fix it
consistently.

> There is an rsync server running continuously on the bileto
> instance, and snakefruit polls it every 5 minutes looking for a new
> file to read. Bileto doesn't "start" rsync.

So I suggest using a lock file (in both the rsync process and bileto)
to ensure to not run rsync while bileto is updating that file tree.
Another common pattern is to do something like this:

  cp -al cur_tree new_tree
  <change new_tree>
  mv cur_tree old_tree; mv new_tree cur_tree

You can't atomically rename an existing (non-empty) directory, but at
least this is fairly quick and thus will work in almost every case.
However, lock files are cheaper, simpler, and completely robust.

> As I said this was decided by infinity and I'm not sure what would be correct.

Please ask him again what this should mean and add a big fat comment
about it. The concept of "upload flashplugin-nonfree into universe"
simply does not make sense -- asking questions about it might be some
weird LP quirk, but it should at least be documented. You yourself
don't even know what it does any more :-)

[Robert Bruce Park (robru)]

On Aug 22, 2016 10:15 PM, "Martin Pitt" <email address hidden> wrote:
>
> Robert Bruce Park [2016-08-22 16:06 -0000]:
> > Can you elaborate what you mean by "recursively call the method on its
direct children"?
>
> Well, I don't know what this is doing, but you said that __getattr__()
> has some magic to do that kind of iteration.

So a ticket can have many packages. When you "build a ticket" you want to
build each package. The manager class exists to ensure the classes
representing the packages are correctly instantiated and then passes
messages back and forth. So when you call manager.build() you're really
calling .build() on every package instance. Manager.__gettattr__() contains
the logic for calling the method, in parallel, on each instance.

In some cases there's some "global" logic that operates once on the whole
ticket rather than per package, and that's where you see those manager
methods that call __getattr__, they're processing the return values that
came back from each package, etc. But there are other cases where you might
call "manager.foo()" and foo doesn't exist at all, it goes straight to
package.foo() on each package. The idea was that the manager is a sort of
transparent wrapper around the package instances, you can call package
methods on it and it passes the calls down to each instance. Eg, it's an
explicit design goal that you would often call methods on the manager
instance that do not exist, which is why __getattr__ is defined. It's just
unfortunate that so many methods require a bit of intervention before /
after the __getattr__ logic that you see so many methods calling
__gettattr__ with their own method name.

>
> > The reason that __getattr__ is defined to pass methods down to
> > children is that this is called from dozens of places (and many of
> > those cases it isn't overloaded at all, it's just a direct pass
> > through), so I'm not going to reimplement that in every single place
> > that needs to pass through. I'll probably rename __getattr__() to
> > call_on_all_packages().
>
> Doing "__getattr__ = call_on_all_packages" and replacing half of the
> calls with the other makes things even *more*
> confusing/implicit/surprising, so better revert that bit or fix it
> consistently.

What do you mean replacing half the calls? Nothing is calling
"self.__getattr__()" any longer so it's perfectly consistent. __getattr__
is kept for the cases when a nonexistent method is called on the manager
class. That has to stay because they chain together, eg, you can call
"manager.foo().bar()" and that calls foo and bar on all package instances.
Getting rid of __getattr__ would result in code like
"manager.call_on_all_packages('foo').call_on_all_packages('bar')", eg,
redundant slop.

>
> > There is an rsync server running continuously on the bileto
> > instance, and snakefruit polls it every 5 minutes looking for a new
> > file to read. Bileto doesn't "start" rsync.
>
> So I suggest using a lock file (in both the rsync process and bileto)
> to ensure to not run rsync while bileto is updating that file tree.
> Another common pattern is to do something like this:
>
> cp -al cur_tree new_tree
> <change new_tree>
> mv cur_t...

=== modified file 'Makefile'
--- Makefile	2016-07-11 21:41:48 +0000
+++ Makefile	2016-08-22 16:49:27 +0000
@@ -31,8 +31,10 @@
 
 install: install-common-deps
 	# Need bzr-builddeb for merging debian/changelogs.
-	$(apt) uuid tree devscripts patchutils diffstat quilt libdistro-info-perl fakeroot dput python3-yaml python3-amqplib python3-psycopg2 python3-gevent gunicorn3 supervisor postgresql-client python-swiftclient bzr-builddeb
+	$(apt) rsync uuid tree devscripts patchutils diffstat quilt libdistro-info-perl fakeroot dput python3-yaml python3-amqplib python3-psycopg2 python3-gevent gunicorn3 supervisor postgresql-client python-swiftclient bzr-builddeb
 	$(envsubst) <files/supervisor.conf >/etc/supervisor/conf.d/bileto.conf
+	cp files/rsyncd.conf /etc/rsync-juju.d/020-bileto.conf
+	service rsync restart
 	# Branch branches if necessary
 	[ -d "$(britney_path)" ]       || git clone $(britney_branch) $(britney_path)
 	[ -d "$(britney_hints_path)" ] || bzr branch $(britney_hints) $(britney_hints_path)

=== modified file 'bileto/actions.py'
--- bileto/actions.py	2016-07-08 17:57:20 +0000
+++ bileto/actions.py	2016-08-22 16:49:27 +0000
@@ -15,6 +15,11 @@
         finalize='Merging to trunk.',
         abandon='Abandoning ticket.',
     )
+    options = dict(
+        publish=dict(
+            ACK_PACKAGING='Check this option if you have reviewed and '
+            'approved all the packaging diffs for this ticket.'),
+    )
 
     @staticmethod
     def status(manager):
@@ -39,8 +44,7 @@
     @staticmethod
     def publish(manager):
         """Copy Packages to Destination Archive"""
-        manager.checkstatus().validate().checkupload().ackaging()
-        manager.checkdest().unapproved().unbuilt().publish()
+        manager.validate().prepublish().publish()
 
     @staticmethod
     def finalize(manager):

=== modified file 'bileto/static/index.html'
--- bileto/static/index.html	2016-08-04 17:07:10 +0000
+++ bileto/static/index.html	2016-08-22 16:49:27 +0000
@@ -229,7 +229,7 @@
 <td ng-if="!editing"><a href="/log/{{req.request_id}}/build" target="_blank">Build</a></td>
 <td ng-if="req.siloname && !editing"><a href="/log/{{req.request_id}}/diff" target="_blank">Diff</a></td>
 <td ng-if="!editing"><a href="/log/{{req.request_id}}/revert" target="_blank">Revert</a></td>
-<td ng-if="req.siloname && !editing"><a href="{{meta.jenkins}}/job/{{req.siloname.replace('/', '-')}}-2-publish/build" target="_blank">Publish</a></td>
+<td ng-if="req.siloname && !editing"><a href="/log/{{req.request_id}}/publish" target="_blank">Publish</a></td>
 <td ng-if="!editing"><a href="/log/{{req.request_id}}/finalize" target="_blank">Finalize</a></td>
 <td ng-if="!editing"><a href="/log/{{req.request_id}}/abandon" target="_blank">Abandon</a></td>
 <td ng-if="editing"><a ng-click="cancel()">Cancel Edit</a></td>

=== modified file 'bileto/streams.py'
--- bileto/streams.py	2016-08-16 16:36:52 +0000
+++ bileto/streams.py	2016-08-22 16:49:27 +0000
@@ -127,6 +127,7 @@
         post = join(request.path, 'latest/')
         packages = list_packages(action, ticket)
         message = getattr(Actions, action).__doc__
+    options = Actions.options.get(action, {})
     leaf = 'success_*False' if failures else 'info'
     paths = glob(join(Config.HOME, 'log', str(ticket), action, '[0-9]*', leaf))
     meta = dict()
@@ -156,6 +157,8 @@
         REQUESTID=str(ticket),
         TEAMS=COMMA.join(session.get('teams', [])),
     )
+    for key in Actions.options.get(action, {}):
+        env[key] = str(request.form.get(key))
     path = join(Config.ROOT, 'bileto', 'worker', 'main.py')
     Popen((path,), env=env)
     sleep(5)

=== modified file 'bileto/templates/loglist.html'
--- bileto/templates/loglist.html	2016-07-30 00:09:36 +0000
+++ bileto/templates/loglist.html	2016-08-22 16:49:27 +0000
@@ -8,6 +8,9 @@
 <div id="trigger">
 {% if session.nickname %}
 <form action="{{ post }}" method="post">
+{% for option, description in options.items() %}
+<p><label><input type="checkbox" name="{{ option }}" value="{{ option }}"><b>{{ option }}:</b> {{ description }}</label></p>
+{% endfor %}
 {% if packages %}
 <p>Include these packages (leave blank to build all):</p>
 <p>

=== modified file 'bileto/worker/manager.py'
--- bileto/worker/manager.py	2016-08-17 20:59:59 +0000
+++ bileto/worker/manager.py	2016-08-22 16:49:27 +0000
@@ -12,7 +12,7 @@
 
 import re
 
-from os import environ
+from os import environ, makedirs, rename
 from os.path import getmtime, join
 
 from time import time
@@ -57,6 +57,7 @@
 SLASH = '/'
 SPACE = ' '
 EMPTY = ''
+DASH = '-'
 PLUS = '+'
 N = '\n'
 
@@ -205,8 +206,8 @@
         pretty_instances(Package.instances.items())
         return Package.instances
 
-    def __getattr__(self, attr):
-        """Pass method calls down to all instances."""
+    def call_on_all_packages(self, attr):
+        """Pass method calls down to all Package instances."""
         def passthrough(*args, **kwargs):
             """Pass arguments down to methods."""
             log.abort_if_cancelled()
@@ -216,6 +217,7 @@
                 for name, package in self.packages().items()])
             return self
         return passthrough
+    __getattr__ = call_on_all_packages
 
     def assign(self):
         """Assign a PPA to this ticket, if possible."""
@@ -237,20 +239,20 @@
             raise BiletoError('Reverting MPs is not supported.')
         self.manual_class = Revert
         log.vars(self.types(), 'manager.types()')
-        return self.__getattr__('collect')()
+        return self.call_on_all_packages('collect')()
 
     def upload(self):
         """Prevent cancellation once uploading begins."""
         log.cancellable = False
         log.info('Point of no return! Job can no longer be cancelled.')
-        return self.__getattr__('upload')()
+        return self.call_on_all_packages('upload')()
 
     def diff(self):
         """Force diffing of all packages."""
         environ.update(INCLUDES=self.ticket.sources)
         for func in (Manager.names, Manager.types, Manager.packages):
             func.cache_clear()
-        self.__getattr__('diff')()
+        self.call_on_all_packages('diff')()
         set_swift_creds()
         stdout = parallelize([VCS.swift_upload_()])[0]
         log.info('Uploaded diffs:\n%s', stdout)
@@ -262,7 +264,7 @@
         assert_lockfile_age(self.ticket.lockfile)
         lock = open('{}-status'.format(self.ticket.lockfile), 'w')
         flock(lock, LOCK_EX | LOCK_NB)
-        self.__getattr__('status')()
+        self.call_on_all_packages('status')()
         assert_lockfile_age(self.ticket.lockfile)
         states = defaultdict(list)
         versions = defaultdict(dict)
@@ -284,6 +286,30 @@
             Actions.finalize(self)
         return self
 
+    def prepublish(self):
+        """Abort publication if any safety checks failed."""
+        self.call_on_all_packages('prepublish')()
+        msgs = defaultdict(list)
+        for source_id, message in sorted(self.retvals):
+            if message:
+                msgs[message].append(source_id)
+        if msgs:
+            raise BiletoError('Publish failed: {}'.format(pretty_states(msgs)))
+        return self
+
+    def publish(self):
+        """Write package manifest to disk."""
+        self.call_on_all_packages('publish')()
+        filename = 'packagelist_rsync_{}'.format(
+            self.ticket.siloname.replace(SLASH, DASH))
+        tmpfile = join(Config.HOME, filename)
+        lines = sorted(Package.manifest)
+        log.vars(lines, 'Manifest lines')
+        with open(tmpfile, 'w', encoding='utf-8') as manifest:
+            manifest.writelines(lines)
+        makedirs(join(Config.HOME, 'publish'), exist_ok=True)
+        rename(filename, join(Config.HOME, 'publish', filename))
+
     def nuke(self, status):
         """Delete PPA contents."""
         # TODO: Eventually delete ephemeral PPAs here

=== modified file 'bileto/worker/manual.py'
--- bileto/worker/manual.py	2016-03-10 01:47:08 +0000
+++ bileto/worker/manual.py	2016-08-22 16:49:27 +0000
@@ -3,6 +3,8 @@
 Defines steps unique to manually-built source packages.
 """
 
+from os import environ
+
 from asyncio import coroutine
 
 from bileto.worker.package import Package
@@ -26,3 +28,12 @@
     @coroutine
     def expose(self):
         """NOP; Users will expose the source package themselves."""
+
+    @coroutine
+    def check_package_ack(self, source=None):
+        """User must **always** have upload rights for manual sources."""
+        if not self.authorized:
+            return '{} not authorized to upload {}'.format(
+                environ['BLAME'], self.name)
+        result = yield from super().check_package_ack(source=source)
+        return result

=== modified file 'bileto/worker/merge.py'
--- bileto/worker/merge.py	2016-07-28 20:10:13 +0000
+++ bileto/worker/merge.py	2016-08-22 16:49:27 +0000
@@ -98,6 +98,7 @@
     """Define the steps unique to packages built from merge proposals."""
     all_mps = {}
     target = None
+    acceptable = BUILDABLE_STATES
 
     @classmethod
     def prepare_all(cls, urls):
@@ -163,14 +164,14 @@
             return 'PPA/bzr version mismatch'
 
     @coroutine
-    def check_merge_states(self, source=None, acceptable=BUILDABLE_STATES):
+    def check_merge_states(self, source=None):
         """Check that merges are in acceptable states."""
         # (\/) (^,,,^) (\/)
         your_merges_are_bad_and_you_should_feel_bad = False
         for merge in self.merges:
             url = getattr(merge, 'web_link', str(merge))
             state = getattr(merge, 'queue_status', 'Not even a merge')
-            if state not in acceptable:
+            if state not in self.acceptable:
                 log.error('%s: %s', state, url)
                 your_merges_are_bad_and_you_should_feel_bad = True
         if your_merges_are_bad_and_you_should_feel_bad:
@@ -268,3 +269,10 @@
     def push(self):
         """Push our final release to target trunk."""
         yield from VCS.push_to_trunk(self.target, **self.env)
+
+    @coroutine
+    def prepublish(self):
+        """Change check_merge_states to consider what is publishable."""
+        self.acceptable = PUBLISHABLE_STATES
+        result = yield from super().prepublish()
+        return result

=== modified file 'bileto/worker/package.py'
--- bileto/worker/package.py	2016-08-15 16:38:33 +0000
+++ bileto/worker/package.py	2016-08-22 16:49:27 +0000
@@ -3,27 +3,45 @@
 Defines steps common to all types of packages that we handle.
 """
 
+from os import environ
 from os.path import join
+
 from contextlib import suppress
 from asyncio import coroutine, gather
 
+from lazr.restfulclient.errors import ClientError
+
 from bileto.settings import Config
 from bileto.models import BUILD_FAILURES, BUILD_STATES
 
 from bileto.worker.vcs import VCS
 from bileto.worker.log import log
+from bileto.worker.version import V
 
+DIFF = '{}_{}_{{}}.diff'
 SLASH = '/'
 EMPTY = ''
+TAB = '\t'
 N = '\n'
 
 
+@coroutine
+def run_checks(checks, *args, **kwargs):
+    """Run checks and return first message found."""
+    for check in checks:
+        message = yield from check(*args, **kwargs)
+        if message:
+            return message
+
+
 # pylint: disable=too-many-instance-attributes
 class Package:
     """Represent the steps necessary to build & publish a package."""
+    authorized = None
     artifacts = ''
     published = None
     instances = {}
+    manifest = []
 
     def __init__(self, name, distro, series, dest, ppa):
         self.id = join(series.name, name)
@@ -32,9 +50,14 @@
         self.name = name
         self.dest = dest
         self.ppa = ppa
+        diff = DIFF.format(series.name, name)
+        self.content_diff = diff.format('content')
+        self.packaging_diff = diff.format('packaging_changes')
         self.env = dict(
             SOURCE=name,
             SERIES=series.name,
+            CONTENT_DIFF=self.content_diff,
+            PACKAGING_DIFF=self.packaging_diff,
         )
         self.instances[self.id] = self
 
@@ -52,10 +75,7 @@
             self.check_diff_exists,
             self.check_destination_version,
         )
-        for check in checks:
-            message = yield from check(source=source)
-            if message:
-                break
+        message = yield from run_checks(checks, source=source)
         return (self.id, message or 'Successfully built', self.published)
 
     @coroutine
@@ -85,7 +105,7 @@
     def check_archive_pocket(self, source):
         """Check if our package is in any archive pocket."""
         version = source.source_package_version
-        dest = self.get_source(self.dest, version)
+        dest = self.get_source(self.dest, version=version)
         if dest:
             results = yield from gather(
                 self.archive_id(source), self.archive_id(dest))
@@ -148,8 +168,7 @@
     @coroutine
     def check_diff_exists(self, source):
         """Ensure that this package has a diff in the ticket artifacts."""
-        diff_path = '{}_{}_content.diff'.format(self.series.name, self.name)
-        if diff_path not in self.artifacts:
+        if self.content_diff not in self.artifacts:
             return 'Diff missing'
 
     @coroutine
@@ -157,6 +176,37 @@
         """Not built from merges; skipping changelog check."""
 
     @coroutine
+    def check_upload_rights(self, source):
+        """Ensure publisher has permission to upload."""
+        blame = environ['BLAME']
+        main = self.get_source(self.series.main_archive, pocket='Release')
+        # Default to flashplugin-nonfree/universe in case this package is NEW
+        kwargs = dict(
+            distroseries=self.series,
+            sourcepackagename='flashplugin-nonfree',
+            component='universe',
+            pocket='Proposed',
+            person=blame)
+        if main:
+            kwargs.update(
+                component=main.component_name,
+                sourcepackagename=self.name)
+        log.vars(kwargs, 'calling checkUpload')
+        try:
+            self.series.main_archive.checkUpload(**kwargs)
+            self.authorized = True
+        except ClientError:
+            self.authorized = False
+        log.debug('Can %s upload %s? %s', blame, self.name, self.authorized)
+
+    @coroutine
+    def check_package_ack(self, source):
+        """Abort publication if packaging diffs have not been acknowledged."""
+        if self.packaging_diff in self.artifacts:
+            if 'ACK_PACKAGING' not in environ or not self.authorized:
+                return 'Packaging diff requires ACK'
+
+    @coroutine
     def build(self):
         """Build source package."""
         yield from VCS.build_(**self.env)
@@ -177,23 +227,21 @@
     def expose(self):
         """NOP; Subclasses may optionally implement this."""
 
-    def get_sources(self, archive, version=None):
+    def get_sources(self, archive, **kwargs):
         """Get a list of lplib objects representing versions in archive."""
-        kwargs = dict(
+        kwargs.update(
             distro_series=self.series,
             source_name=self.name,
             order_by_date=True,
             exact_match=True)
-        if version:
-            kwargs.update(version=version)
         return [
             source for source in archive.getPublishedSources(**kwargs)
             if source.status != 'Deleted']
 
-    def get_source(self, archive, version=None):
+    def get_source(self, archive, **kwargs):
         """Get the lplib object representing this source."""
         with suppress(IndexError):
-            return self.get_sources(archive, version)[0]
+            return self.get_sources(archive, **kwargs)[0]
 
     def find_dsc_url(self, source):
         """Identify the URL to a DSC file for this package in an archive."""
@@ -214,11 +262,73 @@
             COMPONENT=dest_source.component_name if dest_source else 'NEW',
         )
         dest_dsc = self.find_dsc_url(dest_source or self.get_source(
-            self.dest.distribution.main_archive))
+            self.series.main_archive))
         yield from VCS.diff_dest_(DSC_URL=dest_dsc, **self.env)
         log.info('Diffed %s.', self.id)
 
     @coroutine
+    def prepublish(self):
+        """Abort publication if problems discovered."""
+        source = self.get_source(self.ppa)
+        checks = (
+            self.check_ppa_version,
+            self.check_build_status,
+            self.check_diff_exists,
+            self.check_upload_rights,
+            self.check_package_ack,
+            self.check_destination_version,
+            self.check_merge_states,
+            self.check_new_commits,
+        )
+        message = yield from run_checks(checks, source=source)
+        return (self.id, message)
+
+    def copy_to_dest(self, source):
+        """Copy this source package to the destination PPA."""
+        log.info('Copying %s to %s.', source.display_name, self.dest.web_link)
+        self.dest.copyPackage(
+            from_archive=self.ppa,
+            from_pocket=source.pocket,
+            from_series=self.series.name,
+            include_binaries=True,
+            to_pocket='Release',
+            to_series=self.series.name,
+            source_name=self.name,
+            version=source.source_package_version)
+
+    def write_manifest(self, source, dest_version):
+        """Add a line in the manifest representing this package."""
+        distro, series = source.distro_series_link.split(SLASH)[-2:]
+        self.manifest.append(TAB.join([
+            SLASH.join([Config.ppa_team, distro, self.ppa.name]),
+            source.pocket,
+            series,
+            'Proposed',
+            series,
+            self.name,
+            source.source_package_version,
+            dest_version,
+            environ['BLAME'],
+            distro,
+        ]) + N)
+
+    @coroutine
+    def publish(self):
+        """Publish packages from PPA to dest archive."""
+        source = self.get_source(self.ppa)
+        dest = self.get_source(self.dest)
+        dest_version = dest.source_package_version if dest else '0'
+        if V(source.source_package_version) > dest_version:
+            if self.dest.self_link == self.series.main_archive_link:
+                self.write_manifest(source, dest_version)
+            else:
+                self.copy_to_dest(source)
+        else:
+            log.warning('Source: %s', source.display_name)
+            log.warning('Dest:   %s', dest.display_name)
+            log.warning('Source version not higher than dest, skipping!')
+
+    @coroutine
     def merge(self):
         """NOP; implemented by subclasses."""
 

=== added file 'files/rsyncd.conf'
--- files/rsyncd.conf	1970-01-01 00:00:00 +0000
+++ files/rsyncd.conf	2016-08-22 16:49:27 +0000
@@ -0,0 +1,7 @@
+[publish]
+comment = Bileto Publication Data
+path = /var/www/publish/
+read only = no
+list = yes
+uid = www-data
+group = www-data

=== modified file 'scripts/vcs.sh'
--- scripts/vcs.sh	2016-08-17 19:13:08 +0000
+++ scripts/vcs.sh	2016-08-22 16:49:27 +0000
@@ -205,20 +205,18 @@
     cd "$WORKPARENT"
     our_dsc="$(echo ${SOURCE}_*.dsc)"
     [ -f "$our_dsc" ] || die_gently "Diff failed: No DSC found, was it ever built?"
-    content="${SERIES}_${SOURCE}_content.diff"
-    packaging="${SERIES}_${SOURCE}_packaging_changes.diff"
-    raw="$packaging.raw"
-    touch "$content"
+    raw="$PACKAGING_DIFF.raw"
+    touch "$CONTENT_DIFF"
     if [ -n "$DSC_URL" ]; then
         download_from_dsc "$DSC_URL"
         dest_dsc="$(basename "$DSC_URL")"
-        debdiff_wrapper "$dest_dsc" "$our_dsc" "$content"
-        stdout filterdiff --clean --include '*debian/*' --exclude '*changelog' <"$content" >"$raw" || die "Failed to discover packaging changes."
+        debdiff_wrapper "$dest_dsc" "$our_dsc" "$CONTENT_DIFF"
+        stdout filterdiff --clean --include '*debian/*' --exclude '*changelog' <"$CONTENT_DIFF" >"$raw" || die "Failed to discover packaging changes."
     else
         echo '---NEW SOURCE PACKAGE---' >"$raw"
     fi
     if [ -s "$raw" ]; then
-        _build_packaging_diff "$content" "$raw" >"$packaging"
+        _build_packaging_diff "$CONTENT_DIFF" "$raw" >"$PACKAGING_DIFF"
     fi
 }
 

=== modified file 'tests/test_actions.py'
--- tests/test_actions.py	2016-06-30 18:01:09 +0000
+++ tests/test_actions.py	2016-08-22 16:49:27 +0000
@@ -45,14 +45,9 @@
         manager = Mock()
         Actions.publish(manager)
         self.assertSequenceEqual(manager.mock_calls, [
-            call.checkstatus(),
-            call.checkstatus().validate(),
-            call.checkstatus().validate().checkupload(),
-            call.checkstatus().validate().checkupload().ackaging(),
-            call.checkdest(),
-            call.checkdest().unapproved(),
-            call.checkdest().unapproved().unbuilt(),
-            call.checkdest().unapproved().unbuilt().publish(),
+            call.validate(),
+            call.validate().prepublish(),
+            call.validate().prepublish().publish(),
         ])
 
     def test_actions_finalize(self):

=== modified file 'tests/test_streams.py'
--- tests/test_streams.py	2016-07-29 21:10:56 +0000
+++ tests/test_streams.py	2016-08-22 16:49:27 +0000
@@ -178,6 +178,24 @@
         self.assertEqual(env['ACTION'], 'build')
         self.assertEqual(env['REQUEST_PATH'], '/log/1/build/latest/')
 
+    @patch('bileto.streams.Popen')
+    def test_run_subprocess_form_env(self, popen):
+        """Ensure forms are converted to env vars safely."""
+        ret = self.app.post('/log/1/publish/latest/', data=dict(
+            ACK_PACKAGING='ACK_PACKAGING',
+            MALICIOUS_VAR='REMOTE CODE EXPLOIT HERE',
+        ))
+        self.assertIn(b'Redirecting...', ret.data)
+        self.assertEqual(ret.status_code, 303)
+        env = popen.mock_calls[0][2]['env']
+        self.assertSequenceEqual(popen.mock_calls, [
+            call((join(Config.ROOT, 'bileto', 'worker', 'main.py'),), env=env),
+        ])
+        self.assertEqual(env['ACTION'], 'publish')
+        self.assertEqual(env['REQUEST_PATH'], '/log/1/publish/latest/')
+        self.assertEqual(env['ACK_PACKAGING'], 'ACK_PACKAGING')
+        self.assertNotIn('MALICIOUS_VAR', env)
+
     def test_cancel(self):
         """Test that jobs can be cancelled."""
         root = join(self.tempdir, 'log')

=== modified file 'tests/test_worker_manager.py'
--- tests/test_worker_manager.py	2016-08-17 20:59:59 +0000
+++ tests/test_worker_manager.py	2016-08-22 16:49:27 +0000
@@ -129,8 +129,8 @@
 
     @patch('bileto.worker.manager.Manager.names')
     @patch('bileto.worker.manager.Manager.merges')
-    @patch('bileto.worker.manager.Manager.__getattr__')
-    def test_manager_types_revert(self, attr, merges, names):
+    @patch('bileto.worker.manager.Manager.call_on_all_packages')
+    def test_manager_types_revert(self, call_on_all, merges, names):
         """Correctly choose what revert types to build with."""
         merges.return_value = dict()
         names.return_value = {'foo', 'bar'}
@@ -141,12 +141,12 @@
             Merge: set(),
             Revert: {'foo', 'bar'},
         })
-        attr.assert_called_once_with('collect')
+        call_on_all.assert_called_once_with('collect')
 
     @patch('bileto.worker.manager.Manager.names')
     @patch('bileto.worker.manager.Manager.merges')
-    @patch('bileto.worker.manager.Manager.__getattr__')
-    def test_manager_types_revert_no_MPs(self, attr, merges, names):
+    @patch('bileto.worker.manager.Manager.call_on_all_packages')
+    def test_manager_types_revert_no_MPs(self, call_on_all, merges, names):
         """Do not attempt to revert merges."""
         merges.return_value = dict(foo=[Mock()])
         names.return_value = {'foo', 'bar'}
@@ -257,14 +257,14 @@
             call.validate(),
         ])
 
-    @patch('bileto.worker.manager.Manager.__getattr__')
-    def test_manager_upload(self, getattr_mock):
+    @patch('bileto.worker.manager.Manager.call_on_all_packages')
+    def test_manager_upload(self, call_on_all_packages):
         """Prevent cancellation after uploading."""
         log.cancellable = True
         ticket = Mock()
         manager = Manager(ticket)
         manager.upload()
-        getattr_mock.assert_called_once_with('upload')
+        call_on_all_packages.assert_called_once_with('upload')
         assert not log.cancellable
 
     @patch('bileto.worker.manager.Package')
@@ -311,7 +311,9 @@
             ('xenial/media-hub', 'Successfully built', None),
             ('yakkety/media-hub', 'Successfully built', None),
         ]
-        ticket = Mock(status='Preparing packages.')
+        ticket = Mock(
+            status='Preparing packages.',
+            lockfile=join(self.tempdir, 'lockfile'))
         manager = Manager(ticket)
         manager.status()
         ticket.set_status.assert_called_once_with(
@@ -347,7 +349,9 @@
             ('xenial/media-hub', 'Successfully built', None),
             ('yakkety/media-hub', 'Successfully built', None),
         ]
-        ticket = Mock(status='Preparing packages.')
+        ticket = Mock(
+            status='Preparing packages.',
+            lockfile=join(self.tempdir, 'lockfile'))
         manager = Manager(ticket)
         manager.status()
         ticket.set_status.assert_called_once_with(
@@ -368,7 +372,9 @@
             ('xenial/media-hub', 'Release pocket', '2'),
             ('yakkety/media-hub', 'Updates pocket', '3'),
         ]
-        ticket = Mock(status='Preparing packages.')
+        ticket = Mock(
+            status='Preparing packages.',
+            lockfile=join(self.tempdir, 'lockfile'))
         manager = Manager(ticket)
         manager.status()
         ticket.set_status.assert_called_once_with(
@@ -378,6 +384,60 @@
             '"2", "yakkety": "3"}}')
         Acts.finalize.assert_called_once_with(manager)
 
+    @patch('bileto.worker.manager.parallelize')
+    @patch('bileto.worker.manager.Manager.packages')
+    def test_manager_prepublish(self, packages, parallelize):
+        """Allow publish to proceed."""
+        packages.return_value = {}
+        parallelize.return_value = [
+            ('vivid/media-hub', None),
+            ('xenial/media-hub', None),
+            ('yakkety/media-hub', None),
+        ]
+        ticket = Mock()
+        manager = Manager(ticket)
+        self.assertEqual(manager.prepublish(), manager)  # No exception raised
+
+    @patch('bileto.worker.manager.parallelize')
+    @patch('bileto.worker.manager.Manager.packages')
+    def test_manager_prepublish_abort(self, packages, parallelize):
+        """Prevent publication when problems found."""
+        packages.return_value = {}
+        parallelize.return_value = [
+            ('vivid/media-hub', 'Diff missing'),
+            ('xenial/media-hub', 'Diff missing'),
+            ('yakkety/media-hub', 'Dependency wait'),
+        ]
+        ticket = Mock()
+        manager = Manager(ticket)
+        with self.assertRaisesRegex(BiletoError, 'Publish failed: Dep'):
+            manager.prepublish()
+
+    @patch('bileto.worker.manager.rename')
+    @patch('bileto.worker.manager.makedirs')
+    @patch('bileto.worker.manager.parallelize')
+    @patch('bileto.worker.manager.Manager.packages')
+    @patch('bileto.worker.manager.Package.manifest', ['a\n', 'b\n', 'c\n'])
+    def test_manager_publish(self, packages, parallelize, makedirs, rename):
+        """Do publishy things."""
+        packages.return_value = {}
+        parallelize.return_value = [
+            ('vivid/media-hub', None),
+            ('xenial/media-hub', None),
+            ('yakkety/media-hub', None),
+        ]
+        ticket = Mock(siloname='ubuntu/landing-555')
+        manager = Manager(ticket)
+        manager.publish()
+        makedirs.assert_called_once_with(
+            join(self.tempdir, 'publish'), exist_ok=True)
+        filename = 'packagelist_rsync_ubuntu-landing-555'
+        tmpfile = join(self.tempdir, filename)
+        path = join(self.tempdir, 'publish', filename)
+        rename.assert_called_once_with(filename, path)
+        with open(tmpfile, encoding='utf-8') as data:
+            self.assertEqual(data.read().strip(), 'a\nb\nc')
+
     @patch('bileto.worker.manager.Manager.ppa')
     @patch('bileto.worker.vcs.create_subprocess_exec', POST_NUKE)
     def test_manager_nuke(self, ppa):

=== added file 'tests/test_worker_manual.py'
--- tests/test_worker_manual.py	1970-01-01 00:00:00 +0000
+++ tests/test_worker_manual.py	2016-08-22 16:49:27 +0000
@@ -0,0 +1,28 @@
+"""Bileto Manual Test
+
+Test that manual packages behave correctly.
+"""
+
+from os import environ
+
+from unittest.mock import patch
+
+from tests.tests import WorkerTestCase
+
+from bileto.worker.manual import Manual
+
+
+class ManualTestCase(WorkerTestCase):
+    """Test Manual Packages."""
+
+    @patch('bileto.worker.package.Package.check_package_ack')
+    def test_manual_check_package_ack(self, check_package_ack):
+        """Require upload rights for all manual sources."""
+        environ['BLAME'] = 'robru'
+        manual = Manual('oxide', self.distro, self.series, self.dest, self.ppa)
+        manual.authorized = True
+        self.assertEqual(self.run_coro(manual.check_package_ack()), None)
+        manual.authorized = False
+        self.assertEqual(
+            self.run_coro(manual.check_package_ack()),
+            'robru not authorized to upload oxide')

=== modified file 'tests/test_worker_merge.py'
--- tests/test_worker_merge.py	2016-07-21 17:10:54 +0000
+++ tests/test_worker_merge.py	2016-08-22 16:49:27 +0000
@@ -11,7 +11,7 @@
 from tests.tests import WorkerTestCase, fake_subproc
 
 from bileto.settings import Config
-from bileto.worker.merge import Merge
+from bileto.worker.merge import BUILDABLE_STATES, PUBLISHABLE_STATES, Merge
 
 
 STD = (b'', b'')
@@ -309,3 +309,13 @@
             call(join(Config.ROOT, 'scripts', 'vcs.sh'),
                  'lp:ofono', env=env, stdout=PIPE, stderr=PIPE),
         ])
+
+    @patch('bileto.worker.package.Package.prepublish')
+    def test_merge_prepublish(self, prepublish):
+        """Set acceptable merge states for publishing."""
+        Merge.all_mps = dict(ofono=[self.mp])
+        merge = Merge('ofono', self.distro, self.series, self.dest, self.ppa)
+        self.assertEqual(merge.acceptable, BUILDABLE_STATES)
+        self.run_coro(merge.prepublish())
+        self.assertEqual(merge.acceptable, PUBLISHABLE_STATES)
+        prepublish.assert_called_once_with()

=== modified file 'tests/test_worker_package.py'
--- tests/test_worker_package.py	2016-07-21 17:10:54 +0000
+++ tests/test_worker_package.py	2016-08-22 16:49:27 +0000
@@ -3,10 +3,14 @@
 Test the steps for building a package.
 """
 
+from os import environ
 from os.path import join
+
 from subprocess import PIPE
 from unittest.mock import Mock, call, patch
 
+from lazr.restfulclient.errors import ClientError
+
 from tests.tests import WorkerTestCase, fake_subproc
 
 from bileto.settings import Config
@@ -37,6 +41,10 @@
 class PackageTestCase(WorkerTestCase):
     """Test Packages."""
 
+    def setUp(self):
+        super().setUp()
+        Package.manifest.clear()
+
     def test_package_status_ready_to_build(self):
         """Report Ready to build when no package in PPA."""
         self.ppa.getPublishedSources.return_value = []
@@ -243,3 +251,117 @@
             call(source_name='dif', distro_series=self.series,
                  exact_match=True, order_by_date=True),
         ])
+
+    def test_package_prepublish_ready_to_build(self):
+        """Report Ready to build when no package in PPA."""
+        self.ppa.getPublishedSources.return_value = []
+        package = Package('zap', self.distro, self.series, self.dest, self.ppa)
+        self.assertEqual(
+            self.run_coro(package.prepublish()),
+            ('xenial/zap', 'Ready to build'))
+
+    def test_package_prepublish_package_ack_unauthorized(self):
+        """Report when user not authorized to ack."""
+        environ['BLAME'] = 'robru'
+        environ['ACK_PACKAGING'] = 'yes'
+        self.ppa.getPublishedSources.return_value = [
+            Mock(source_package_version='42')]
+        self.dest.getPublishedSources.return_value = []
+        i386 = Mock(arch_tag='i386', buildstate='Successfully built')
+        source = Mock(source_package_version='42')
+        source.getBuilds.return_value = [i386]
+        self.ppa.getPublishedSources.return_value = [source]
+        self.series.main_archive.getPublishedSources.return_value = []
+        self.series.getPackageUploads.return_value = Listish()
+        package = Package('zap', self.distro, self.series, self.dest, self.ppa)
+        package.artifacts = (
+            'time/xenial_zap_content.diff (1 MILLION LINES)\n'
+            'time/xenial_zap_packaging_changes.diff (1 THOUSAND LINES)\n')
+        package.series.main_archive.checkUpload.side_effect = ClientError(0, 0)
+        self.assertEqual(
+            self.run_coro(package.prepublish()),
+            ('xenial/zap', 'Packaging diff requires ACK'))
+        self.assertFalse(package.authorized)
+
+    def test_package_prepublish_package_ack_authorized(self):
+        """Report when user not authorized to ack."""
+        environ['BLAME'] = 'robru'
+        environ['ACK_PACKAGING'] = 'yes'
+        self.ppa.getPublishedSources.return_value = [
+            Mock(source_package_version='42')]
+        self.dest.getPublishedSources.return_value = []
+        i386 = Mock(arch_tag='i386', buildstate='Successfully built')
+        source = Mock(source_package_version='42')
+        source.getBuilds.return_value = [i386]
+        source.getPublishedBinaries.return_value = [
+            Mock(architecture_specific=1, distro_arch_series_link='./i386'),
+        ]
+        self.ppa.getPublishedSources.return_value = [source]
+        self.series.main_archive.getPublishedSources.return_value = [source]
+        self.series.getPackageUploads.return_value = Listish()
+        package = Package('zap', self.distro, self.series, self.dest, self.ppa)
+        package.artifacts = (
+            'time/xenial_zap_content.diff (1 MILLION LINES)\n'
+            'time/xenial_zap_packaging_changes.diff (1 THOUSAND LINES)\n')
+        package.series.main_archive.checkUpload.return_value = True
+        self.assertEqual(
+            self.run_coro(package.prepublish()),
+            ('xenial/zap', None))
+        self.assertTrue(package.authorized)
+
+    @patch('bileto.worker.package.Config.ppa_team', 'glort')
+    def test_package_publish_manifest(self):
+        """Publish a package to ubuntu archive."""
+        environ['BLAME'] = 'robobert'
+        source = Mock(source_package_version='90')
+        source.distro_series_link = 'foo/bar/ubuntu/yakkety'
+        source.pocket = 'Release'
+        self.ppa.getPublishedSources.return_value = [source]
+        dest = Mock(source_package_version='80')
+        self.dest.getPublishedSources.return_value = [dest]
+        self.dest.self_link = self.series.main_archive_link = 'ubuntu archive'
+        package = Package('zap', self.distro, self.series, self.dest, self.ppa)
+        self.run_coro(package.publish())
+        self.assertEqual(package.manifest, [
+            'glort/ubuntu/12321\tRelease\tyakkety\tProposed\tyakkety\tzap\t90'
+            '\t80\trobobert\tubuntu\n'])
+
+    @patch('bileto.worker.package.Config.ppa_team', 'glort')
+    def test_package_publish_copypackage(self):
+        """Publish a package to overlay ppa."""
+        environ['BLAME'] = 'robobert'
+        source = Mock(source_package_version='90')
+        source.distro_series_link = 'foo/bar/ubuntu/yakkety'
+        source.pocket = 'Release'
+        self.ppa.getPublishedSources.return_value = [source]
+        dest = Mock(source_package_version='80')
+        self.dest.getPublishedSources.return_value = [dest]
+        self.series.main_archive_link = 'ubuntu archive'
+        self.dest.self_link = 'some other place'
+        package = Package('zap', self.distro, self.series, self.dest, self.ppa)
+        self.run_coro(package.publish())
+        self.assertEqual(package.manifest, [])
+        self.dest.copyPackage.assert_called_once_with(
+            from_archive=self.ppa,
+            from_pocket='Release',
+            from_series='xenial',
+            include_binaries=True,
+            source_name='zap',
+            to_pocket='Release',
+            to_series='xenial',
+            version='90')
+
+    @patch('bileto.worker.package.Config.ppa_team', 'glort')
+    def test_package_publish_skipped(self):
+        """Skip publishing because version is lower than dest."""
+        environ['BLAME'] = 'robobert'
+        source = Mock(source_package_version='70')
+        source.distro_series_link = 'foo/bar/ubuntu/yakkety'
+        source.pocket = 'Release'
+        self.ppa.getPublishedSources.return_value = [source]
+        dest = Mock(source_package_version='80')
+        self.dest.getPublishedSources.return_value = [dest]
+        package = Package('zap', self.distro, self.series, self.dest, self.ppa)
+        self.run_coro(package.publish())
+        self.assertEqual(package.manifest, [])
+        self.assertEqual(self.dest.copyPackage.mock_calls, [])