launchpad-reviewers team mailing list archive

[Colin Watson <mp+449286@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:publisher-run-parts-site-name into launchpad:master.

Commit message:
Pass SITE_NAME environment variable to run-parts

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/449286

A number of the hook scripts in https://bazaar.launchpad.net/+branch/ubuntu-archive-publishing rely on being able to tell the difference between our different deployments, so that they can behave differently in production and non-production environments.  For example, https://bazaar.launchpad.net/+branch/ubuntu-archive-publishing/view/head:/publish-distro.d/10-sign-releases uses different signing keys for different environments, since of course dogfood doesn't have the production keys.

At the moment, we rely on the `LPCONFIG` variable (implicitly inherited by these hook scripts from the parent process) for this.  However, in a charmed deployment, `LPCONFIG` is set purely based on the charm name, and is the same across production and non-production deployments of the same charm.  As a result, we can't rely on it for this any more.

An approach that we've taken in a few other places is to conditionalize behaviour based on the main site's hostname.  This has the advantage that the condition is spelled using an existing user-visible name (e.g. "launchpad.net") rather than an internal codename that you're just expected to know (e.g. "production" or the various values of `LPCONFIG`).  To support this, I've invented the `SITE_NAME` environment variable, which the publisher now sets to the main site's hostname when running its hook scripts.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:publisher-run-parts-site-name into launchpad:master.

diff --git a/lib/lp/archivepublisher/archivegpgsigningkey.py b/lib/lp/archivepublisher/archivegpgsigningkey.py
index aae8b30..722cecd 100644
--- a/lib/lp/archivepublisher/archivegpgsigningkey.py
+++ b/lib/lp/archivepublisher/archivegpgsigningkey.py
@@ -154,10 +154,13 @@ class SignableArchive:
                 remove_if_exists(output_path)
                 env = {
                     "ARCHIVEROOT": self.pubconf.archiveroot,
+                    "DISTRIBUTION": self.archive.distribution.name,
                     "INPUT_PATH": input_path,
-                    "OUTPUT_PATH": output_path,
                     "MODE": mode.name.lower(),
-                    "DISTRIBUTION": self.archive.distribution.name,
+                    "OUTPUT_PATH": output_path,
+                    # Allow parts to detect whether they're running on
+                    # production.
+                    "SITE_NAME": config.vhost.mainsite.hostname,
                     "SUITE": suite,
                 }
                 run_parts(
diff --git a/lib/lp/archivepublisher/scripts/publish_ftpmaster.py b/lib/lp/archivepublisher/scripts/publish_ftpmaster.py
index b62e3b1..1e94d67 100644
--- a/lib/lp/archivepublisher/scripts/publish_ftpmaster.py
+++ b/lib/lp/archivepublisher/scripts/publish_ftpmaster.py
@@ -26,6 +26,7 @@ from lp.archivepublisher.scripts.publishdistro import PublishDistro
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket, pocketsuffix
 from lp.registry.interfaces.series import SeriesStatus
+from lp.services.config import config
 from lp.services.database.bulk import load_related
 from lp.services.osutils import ensure_directory_exists
 from lp.services.scripts.base import (
@@ -423,6 +424,8 @@ class PublishFTPMaster(LaunchpadCronScript):
         env = {
             "ARCHIVEROOT": archive_config.archiveroot,
             "DISTSROOT": get_backup_dists(archive_config),
+            # Allow parts to detect whether they're running on production.
+            "SITE_NAME": config.vhost.mainsite.hostname,
         }
         if archive_config.overrideroot is not None:
             env["OVERRIDEROOT"] = archive_config.overrideroot
@@ -477,6 +480,8 @@ class PublishFTPMaster(LaunchpadCronScript):
         env = {
             "SECURITY_UPLOAD_ONLY": "yes" if security_only else "no",
             "ARCHIVEROOTS": archive_roots,
+            # Allow parts to detect whether they're running on production.
+            "SITE_NAME": config.vhost.mainsite.hostname,
         }
         run_parts(distribution.name, "finalize.d", log=self.logger, env=env)
 
diff --git a/lib/lp/archivepublisher/tests/test_customupload.py b/lib/lp/archivepublisher/tests/test_customupload.py
index 7a22882..1d50f59 100644
--- a/lib/lp/archivepublisher/tests/test_customupload.py
+++ b/lib/lp/archivepublisher/tests/test_customupload.py
@@ -282,10 +282,11 @@ class TestSigning(TestCaseWithFactory, RunPartsMixin):
             MatchesDict(
                 {
                     "ARCHIVEROOT": Equals(archiveroot),
+                    "DISTRIBUTION": Equals(self.distro.name),
                     "INPUT_PATH": Equals(filename),
-                    "OUTPUT_PATH": Equals("%s.gpg" % filename),
                     "MODE": Equals("detached"),
-                    "DISTRIBUTION": Equals(self.distro.name),
+                    "OUTPUT_PATH": Equals("%s.gpg" % filename),
+                    "SITE_NAME": Equals("launchpad.test"),
                     "SUITE": Equals("suite"),
                 }
             ),
diff --git a/lib/lp/archivepublisher/tests/test_publish_ftpmaster.py b/lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
index 6fd5732..f210112 100644
--- a/lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
+++ b/lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
@@ -530,6 +530,7 @@ class TestPublishFTPMasterScript(
                     "OVERRIDEROOT": Equals(
                         get_archive_root(distro_config) + "-overrides"
                     ),
+                    "SITE_NAME": Equals("launchpad.test"),
                 }
             ),
         )
@@ -595,10 +596,20 @@ class TestPublishFTPMasterScript(
         )
         script.runFinalizeParts(distro)
         _, kwargs = run_parts_fixture.new_value.calls[0]
-        env = kwargs["env"]
-        required_parameters = {"ARCHIVEROOTS", "SECURITY_UPLOAD_ONLY"}
-        missing_parameters = required_parameters.difference(set(env.keys()))
-        self.assertEqual(set(), missing_parameters)
+        distro_config = get_pub_config(distro)
+        self.assertThat(
+            kwargs["env"],
+            ContainsDict(
+                {
+                    "ARCHIVEROOTS": Equals(
+                        "%(root)s %(root)s-partner"
+                        % {"root": get_archive_root(distro_config)}
+                    ),
+                    "SECURITY_UPLOAD_ONLY": Equals("no"),
+                    "SITE_NAME": Equals("launchpad.test"),
+                }
+            ),
+        )
 
     def test_publishSecurityUploads_skips_pub_if_no_security_updates(self):
         script = self.makeScript()
diff --git a/lib/lp/archivepublisher/tests/test_signing.py b/lib/lp/archivepublisher/tests/test_signing.py
index 547c02f..26f6500 100644
--- a/lib/lp/archivepublisher/tests/test_signing.py
+++ b/lib/lp/archivepublisher/tests/test_signing.py
@@ -1624,10 +1624,11 @@ class TestLocalSigningUpload(RunPartsMixin, TestSigningHelpers):
                     "ARCHIVEROOT": Equals(
                         os.path.join(self.temp_dir, self.distro.name)
                     ),
+                    "DISTRIBUTION": Equals(self.distro.name),
                     "INPUT_PATH": Equals(sha256file),
-                    "OUTPUT_PATH": Equals("%s.gpg" % sha256file),
                     "MODE": Equals("detached"),
-                    "DISTRIBUTION": Equals(self.distro.name),
+                    "OUTPUT_PATH": Equals("%s.gpg" % sha256file),
+                    "SITE_NAME": Equals("launchpad.test"),
                     "SUITE": Equals(self.suite),
                 }
             ),