launchpad-reviewers team mailing list archive

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

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/db-bug-752181 into lp:launchpad/db-devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #752181 in Launchpad itself: "publish-distro.d/30-copy-indices relies on unset OVERRIDEROOT"
  https://bugs.launchpad.net/launchpad/+bug/752181

For more details, see:
https://code.launchpad.net/~jtv/launchpad/db-bug-752181/+merge/56903

= Summary =

In replacing the system-specific and distro-specific parts of the cron.publish-ftpmaster script with run-parts plugins, I forgot to pass a parameter from the archives' publisher config to the publish-distro plugins: OVERRIDEROOT.  It was hard-coded in the old script.


== Proposed fix ==

Take OVERRIDEROOT from the archive-configuration model object (it's actually a separate thing from lazr config) and passes it to the script.

== Pre-implementation notes ==

William notes that the indices directory may not exist yet.  So I changed the plugin script to create it if necessary.


== Implementation details ==

I was careless in my unit-testing of the new python-based publish-ftpmaster script.  The two tests for parameters being passed to the plugin scripts would fail if I passed too many, but not if I passed too few (which is what really needs testing).  Proper TDD would have shown me this as I ran the test before implementing the feature.  That wasn't really feasible for such a large and radical branch though; in retrospect it might have been possible to break it down into smaller branches somehow.


== Tests ==

{{{
./bin/test lp.archivepublisher.tests.test_publish_ftpmaster -t passes_parameters
}}}


== Demo and Q/A ==

We run publish-ftpmaster on dogfood and watch for error output or missing results.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  cronscripts/publishing/distro-parts/ubuntu/publish-distro.d/30-copy-indices
  cronscripts/publishing/distro-parts/ubuntu/publish-distro.d/README.txt
  lib/lp/archivepublisher/scripts/publish_ftpmaster.py
  lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
-- 
https://code.launchpad.net/~jtv/launchpad/db-bug-752181/+merge/56903
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/db-bug-752181 into lp:launchpad/db-devel.

=== modified file 'cronscripts/publishing/distro-parts/ubuntu/publish-distro.d/30-copy-indices'
--- cronscripts/publishing/distro-parts/ubuntu/publish-distro.d/30-copy-indices	2011-03-28 09:18:42 +0000
+++ cronscripts/publishing/distro-parts/ubuntu/publish-distro.d/30-copy-indices	2011-04-08 09:29:08 +0000
@@ -2,7 +2,8 @@
 
 echo "$(date -R): Copying the indices into place."
 
-INDICES="$ARCHIVEROOT/indices"
+INDICES=$ARCHIVEROOT/indices
 
+mkdir -p -- $INDICES
 rm -f -- "$INDICES/override"
-cp -- "$OVERRIDEROOT"/override.* "$INDICES/"
+cp -- $OVERRIDEROOT/override.* $INDICES/

=== modified file 'cronscripts/publishing/distro-parts/ubuntu/publish-distro.d/README.txt'
--- cronscripts/publishing/distro-parts/ubuntu/publish-distro.d/README.txt	2011-03-31 08:38:55 +0000
+++ cronscripts/publishing/distro-parts/ubuntu/publish-distro.d/README.txt	2011-04-08 09:29:08 +0000
@@ -18,5 +18,8 @@
 DISTSROOT - the archive's dists root directory
 (e.g. /srv/launchpad.net/ubuntu-archive/ubuntu/dists )
 
+OVERRIDEROOT - the archive's overrides root directory
+(e.g. /srv/launchpad.net/ubuntu-overrides )
+
 The script's PATH will be extended with the Launchpad source tree's
 cronscripts/publishing directory.

=== modified file 'lib/lp/archivepublisher/scripts/publish_ftpmaster.py'
--- lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2011-04-01 07:12:44 +0000
+++ lib/lp/archivepublisher/scripts/publish_ftpmaster.py	2011-04-08 09:29:08 +0000
@@ -256,8 +256,9 @@
         """Execute the publish-distro hooks."""
         archive_config = self.configs[archive.purpose]
         env = {
+            'ARCHIVEROOT': archive_config.archiveroot,
             'DISTSROOT': archive_config.distsroot,
-            'ARCHIVEROOT': archive_config.archiveroot,
+            'OVERRIDEROOT': archive_config.overrideroot,
             }
         self.runParts('publish-distro.d', env)
 

=== modified file 'lib/lp/archivepublisher/tests/test_publish_ftpmaster.py'
--- lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2011-03-31 11:25:53 +0000
+++ lib/lp/archivepublisher/tests/test_publish_ftpmaster.py	2011-04-08 09:29:08 +0000
@@ -479,8 +479,9 @@
         script.runPublishDistroParts(distro.main_archive)
         args, kwargs = script.runParts.calls[0]
         parts_dir, env = args
-        required_parameters = set(["DISTSROOT", "ARCHIVEROOT"])
-        missing_parameters = set(env.keys()).difference(required_parameters)
+        required_parameters = set([
+            "ARCHIVEROOT", "DISTSROOT", "OVERRIDEROOT"])
+        missing_parameters = required_parameters.difference(set(env.keys()))
         self.assertEqual(set(), missing_parameters)
 
     def test_installDists_sets_done_pub(self):
@@ -642,7 +643,7 @@
         args, kwargs = script.runParts.calls[0]
         parts_dir, env = args
         required_parameters = set(["ARCHIVEROOTS", "SECURITY_UPLOAD_ONLY"])
-        missing_parameters = set(env.keys()).difference(required_parameters)
+        missing_parameters = required_parameters.difference(set(env.keys()))
         self.assertEqual(set(), missing_parameters)
 
     def test_publishSecurityUploads_skips_pub_if_no_security_updates(self):