launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

The proposal to merge lp:~jtv/launchpad/db-bug-735621 into lp:launchpad/db-devel has been updated.

Description changed to:

= Summary =

See bug 735621.  This converts an untested soyuz bash script to python.

The script in question is cronscripts/publishing/gen-contents/generate-contents.

In addition to converting the script to python and adding tests, this also breaks the hard-coded tie to Ubuntu and permits the same script to run in parallel for multiple distributions (though not for the same distribution, since it would probably cause trouble).

== Implementation details ==

In the diff you'll see a few templates that are used for writing various configuration scripts that tell the Debian machinery what to do.  These were plain text files with a few replacements made by custom code.  I replaced that with python string interpolation.  The parameters are passed as a dict so they can be accessed by name.  There's no other documentation of how this works or what parameters are passed.  With "bzr log" showing me one change in 2007 and another in 2006 for these files, it's probably more cost-effective for the next engineer to touch this to figure things out to the extent needed than it is to write and maintain documentation.

I couldn't place the script in its old location; it won't import _pythonpath if I put it there.  But I gave it a more descriptive name.

== Test ==

I couldn't find any tests for the old script.  There's only the new test:

{{{
./bin/test -vvc lp.archivepublisher.tests.test_generate_contents_files
}}}

This doesn't cover executing the script itself.  The test is somewhat expensive as it is, at almost 3 seconds; I don't think another 5+ seconds of ZCML parsing will pull their weight.  There's nothing in there but the usual "import and run LaunchpadScript" boilerplate.  I tried it by hand.

A more interesting hole in test coverage is database privileges.  The script barely accesses the database at all, but it calls LpQueryDistro which does.  The archive publisher's database user (as set in the lazr config) should have the required privileges.  Is this worth an expensive extra test?  I'm not sure.

== Demo and Q/A ==

One of the Soyuz gurus will have to verify this.  Apparently the script is so costly that it doesn't even get run on dogfood.  I don't expect to see any changes in performance characteristics.

= Launchpad lint =

The lint is all bogus: files we shouldn't check and one warning we can't avoid.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/config/schema-lazr.conf
  lib/lp/archivepublisher/tests/test_generate_contents_files.py
  lib/lp/archivepublisher/scripts/generate_contents_files.py
  lib/canonical/launchpad/ftests/script.py
  cronscripts/publishing/gen-contents/apt_conf_dist.template
  cronscripts/publishing/gen-contents/Contents.top
  cronscripts/generate-contents-files.py
  cronscripts/publishing/gen-contents/apt_conf_header.template

./lib/canonical/config/schema-lazr.conf
     536: Line exceeds 78 characters.
     619: Line exceeds 78 characters.
     995: Line exceeds 78 characters.
    1084: Line exceeds 78 characters.
./cronscripts/publishing/gen-contents/apt_conf_dist.template
       4: Line exceeds 78 characters.
       5: Line exceeds 78 characters.
./cronscripts/generate-contents-files.py
       8: '_pythonpath' imported but unused

For more details, see:
https://code.launchpad.net/~jtv/launchpad/db-bug-735621/+merge/58321
-- 
https://code.launchpad.net/~jtv/launchpad/db-bug-735621/+merge/58321
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/db-bug-735621 into lp:launchpad/db-devel.