PushApkWorker[1] works locally. Time to put it live! [1] https://github.com/JohanLorenzo/pushapkworker

Here's a first version of PushApkWorker. The exposed configuration is compatible with attachment 8797155 [details]. Its documentation may need enhancements. It also relies on scriptworker 0.7.0, at the moment. I saw 0.8.0 exposes the download piece, which could be useful here. I'd prefer to upgrade it in a a follow up, though. Regarding the direct use of the jarsigner binary, I haven't found any python lib. The examples searchable on GitHub also show a binary wrapper.

Comment on attachment 8797155 [details] Bug 1306307 - Deploy PushApkWorker with Puppet https://reviewboard.mozilla.org/r/82758/#review82220 ::: modules/pushapkworker/templates/config.json.erb:5 (Diff revision 7) > +{ > + "provisioner_id": "<%= scope.lookupvar("config::scriptworker_provisioner_id") %>", > + "worker_group": "<%= scope.lookupvar("config::pushapk_scriptworker_worker_group") %>", > + "worker_type": "<%= scope.lookupvar("config::pushapk_scriptworker_worker_type") %>", > + "worker_id": "<%= @hostname %>", For the record, worker IDs can be longer than 22 characters. My current staging machine is named "dev-linux64-ec2-jlorenzo" (25 characters). Based on `hg grep` worker_id is defined by either: * hostname * fqdn * a hardcoded string I'm not sure what's the best option here. Truncating hostnames doesn't seem like a good idea, either. Is there a way to make puppet to let something like an incremental value, here?

Comment on attachment 8797155 [details] Bug 1306307 - Deploy PushApkWorker with Puppet https://reviewboard.mozilla.org/r/82758/#review82236 I think this works. If you want to proceed with your staging instance, you can hardcode your workerId to a <=22char string. If you want to proceed with a more production-ready instance now, that works as well. ::: manifests/moco-nodes.pp:1167 (Diff revision 7) > include toplevel::server::signingscriptworker > } > > ## Loaners > > +node "dev-linux64-ec2-jlorenzo.dev.releng.use1.mozilla.com" { As mentioned above, we may want to create a permanent location for this for landing, e.g. pushapkworker-1. You can keep dev-linux64-ec2-jlorenzo and use it in your staging puppet env, but for landing it might be good to create that production env.

Comment on attachment 8798047 [details] PushApkWorker (fork of signingscript) I think the below is good to fix before we're fully reliant on this in production as tier1, but doesn't block usage on date. I'm happy to discuss any of it. r+ with a request for followup fixes. README: > # install pushapkworker from pypi > python setup.py develop Nit: `python setup.py develop` installs from the local source tree, rather than pypi, though the dependencies are downloaded from pypi. `pip install pushapkworker` would install pushapkworker from pypi. tox.ini: > usedevelop=True I think @lundjordan found that if you create a `test/__init__.py`, this is no longer necessary. I think `pushapkworker/data/signing_task_schema.json` should be renamed to `pushapk_task_schema.json` or something. `JarSigner`: this can probably be a function. I don't object too strongly to it being an object, but noting that it's an object with one method, and the __init__ function only sets some defaults. ``` try: self.binary_path = context.config['jarsigner_binary'] except KeyError: self.binary_path = 'jarsigner' ``` This could be ``` self.binary_path = context.config.get("jarsigner_binary", "jarsigner") ``` Same with `self.certificate_aliases`. `utils.py` re-defines `download_file`, when it's in scriptworker. I'm ok with this duplication if it's known and conscious. One reason to not import from scriptworker is scriptworker requires python 3.5, but as written, so does `pushapkworker.utils.download_file`. Similarly, `task.py` defines `download_files`, which is similar to `scriptworker.task.download_artifacts`, and `validate_task_schema`, which is similar to `scriptworker.client.validate_json_schema`. Are these duplications intentional? ```python log.info('Downloading APKs...') with aiohttp.ClientSession() as base_ssl_session: orig_session = context.session context.session = base_ssl_session downloaded_apks = await download_files(context) context.session = orig_session ``` In signingscript, I replaced the `context.session` because the signing servers have a special SSL context, and downloading artifacts needed a normal SSL context. I don't think you need this same workaround. Outside of that, [google docstrings](http://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html) and full test coverage would be great as a goal. I know signingscript is currently missing those; it's on my todo list.

Thanks for the detailed reviews Aki! I made the changes on pushapkwoker[1]. For the record, I started the worker with scriptworker 0.6.0 (and not 0.7.0, like I said comment 6), that's why the most of the duplication was there. As the worker uses 0.7.0 anyway, I removed that. I also published the worker on Pypi, just for parity with the README. We're now at 78% of coverage[2]. Testing must be done in script.py, mainly. I'll handle that. Regarding google docstring. I'm not too sure of the end usage here. As our packages are mostly final executable, I wonder what kind of people are willing to import *worker in their own python. [1] https://github.com/mozilla-releng/pushapkworker/compare/336e65738bf1097481d917fe83c53014b72cf240...0.1.1 [2] https://coveralls.io/builds/8217114

The staging configuration should be in a stable state from now on. Here's what basically changed since Aki's review: * A rebase on top of the latest tip (this added changes related to balrogworker and beetmoverworker) * Use mozapkpublisher and pushapkscript (renamed from pushapkworker) 0.1.3 * Make a part of the configuration dependent of the type of the environment I'm gonna now work on the actual production configuration. This will be handled in bug 1307826. Closing this bug, without landing the staging changes onto hg.m.o/build/puppet. Interdiff changes: https://reviewboard.mozilla.org/r/82758/diff/8-14/