Merge ~cjwatson/mojo/+git/subordinates:userdir-ldap into ~mojo-maintainers/mojo/+git/subordinates:master

[Colin Watson]

Add an optional manifest-userdir-ldap

Some deployments want userdir-ldap, which is a reasonable fit here, but isn't wanted everywhere. Add a separate manifest for this as suggested by Tom Haddon in https://portal.admin.canonical.com/C130650.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Tom Haddon (mthaddon)]

One comment line about the archive to use for this.

Also, can you update the README to mention this?

[Colin Watson (cjwatson)]

The inline comment was lost somehow, but you were wondering whether we could use an IS-owned (private) PPA for this. I looked into that, since I'd certainly prefer to use a PPA over CAT. Unfortunately it's rather painful. add-apt-repository only supports public PPAs (https://bugs.launchpad.net/ubuntu/+source/software-properties/+bug/645404), so the only way to do this would be to put explicit username/password credentials in the URL. Those obviously can't be committed to a git repository like this. If at some point there could be a public version of that PPA (IIRC it originally derived from Debian code anyway?), it would be straightforward to switch to that; or it could be configured manually for individual deployments (`juju config userdir-ldap apt-repo-spec` etc.) if it's thought to be worth it. Otherwise I can't really think of a way to do what you request.

I've updated the README as requested.

[Tom Haddon (mthaddon)]

Thanks for the readme update.

I think we could require a secret to be configured in the environment that includes the required credentials (as we do for UA and landscape). We could adapt the check-secrets script to confirm the secrets necessary are there, and then update the bundle line to use the appropriate secrets.

Having said that, I'm not actually sure off the top of my head whether what's needed via the PPA (or -cat) really does need to be secret, or could be moved to a public PPA. I'll follow up on this next week.

[Colin Watson (cjwatson)]

@mthaddon, did you have a chance to follow up on the PPA privacy question, or should I go ahead with sorting out secrets configuration? I'd obviously prefer to avoid that work if we can just make the thing public instead, but otherwise I can deal with it.

[Colin Watson (cjwatson)]

I notice that a not-very-much-older version is already public in https://launchpad.net/~canonical-bootstack/+archive/ubuntu/public/+packages.

[Tom Haddon (mthaddon)]

LGTM, thx

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision a21ca12cce08a4cd0f95bcdb9c08fdde28023ff9

diff --git a/README.md b/README.md
index a628656..a3fbbb0 100644
--- a/README.md
+++ b/README.md
@@ -24,3 +24,8 @@ deployed Juju 2 model:
 If you've already deployed and want to upgrade charms, run:
 
     mojo run -m manifest-upgrade git+lp:~mojo-maintainers/mojo/+git/subordinates subordinates
+
+Some deployments want the userdir-ldap subordinate as well. To deploy that,
+run the following after deploying the common subordinates above:
+
+    mojo run -m manifest-userdir-ldap git+lp:~mojo-maintainers/mojo/+git/subordinates subordinates
diff --git a/bundle-userdir-ldap.yaml b/bundle-userdir-ldap.yaml
new file mode 100644
index 0000000..cd8e1e9
--- /dev/null
+++ b/bundle-userdir-ldap.yaml
@@ -0,0 +1,7 @@
+series: {{ series }}
+applications:
+  userdir-ldap:
+    series: {{ series }}
+    charm: cs:userdir-ldap
+    options:
+      apt-repo-spec: "ppa:canonical-sysadmins/ubuntu/ud-ldap"
diff --git a/charms_to_relate.yaml b/charms_to_relate.yaml
index 1d081d3..49b2586 100644
--- a/charms_to_relate.yaml
+++ b/charms_to_relate.yaml
@@ -21,3 +21,7 @@ telegraf:
       condition-check-output-split-fail-include: status
 ubuntu-advantage:
   install-everywhere: true
+userdir-ldap:
+  install-everywhere: true
+  source-relation: general-info
+  optional: true
diff --git a/manifest-userdir-ldap b/manifest-userdir-ldap
new file mode 100644
index 0000000..e02a3fc
--- /dev/null
+++ b/manifest-userdir-ldap
@@ -0,0 +1,7 @@
+# Confirm the environment is in a good state before proceeding
+include config=manifest-verify
+## Allow `waiting` as a ready state here, because we've just deployed
+## some subordinates, but we're not adding relations until the next step.
+bundle config=bundle-userdir-ldap.yaml additional-ready-states=waiting
+script config=add-relations
+include config=manifest-verify
diff --git a/tests/test_relations.py b/tests/test_relations.py
index 42fef33..454e783 100644
--- a/tests/test_relations.py
+++ b/tests/test_relations.py
@@ -30,7 +30,17 @@ class TestUtilsRelations(unittest.TestCase):
                 "WARNING:root:DRYRUN: would run 'juju relate telegraf:juju-info rabbitmq:juju-info'",
                 "WARNING:root:DRYRUN: would run 'juju relate ubuntu-advantage:juju-info rabbitmq:juju-info'",
             ]
-            utils.relations.process_charms_to_relate(args, ["juju", "relate"], applications, None)
+            utils.relations.process_charms_to_relate(args, ["juju", "relate"], applications, {})
+            self.assertEqual(_logs.output, expected_logs)
+        # Test with the optional userdir-ldap subordinate deployed.
+        with self.assertLogs() as _logs:
+            expected_logs = [
+                "WARNING:root:DRYRUN: would run 'juju relate landscape-client:container rabbitmq:juju-info'",
+                "WARNING:root:DRYRUN: would run 'juju relate telegraf:juju-info rabbitmq:juju-info'",
+                "WARNING:root:DRYRUN: would run 'juju relate ubuntu-advantage:juju-info rabbitmq:juju-info'",
+                "WARNING:root:DRYRUN: would run 'juju relate userdir-ldap:general-info rabbitmq:juju-info'",
+            ]
+            utils.relations.process_charms_to_relate(args, ["juju", "relate"], applications, {"userdir-ldap": {}})
             self.assertEqual(_logs.output, expected_logs)
         # Now test with a charm that defines specific relations.
         applications = (('mysql', ''),)
@@ -41,7 +51,7 @@ class TestUtilsRelations(unittest.TestCase):
                 "WARNING:root:DRYRUN: would run 'juju relate telegraf:mysql mysql:db-admin'",
                 "WARNING:root:DRYRUN: would run 'juju relate ubuntu-advantage:juju-info mysql:juju-info'",
             ]
-            utils.relations.process_charms_to_relate(args, ["juju", "relate"], applications, None)
+            utils.relations.process_charms_to_relate(args, ["juju", "relate"], applications, {})
             self.assertEqual(_logs.output, expected_logs)
         # Test if we're in a container, we shouldn't install ubuntu-advantage.
         _deployed_to_containers.return_value = True
@@ -52,7 +62,7 @@ class TestUtilsRelations(unittest.TestCase):
                 "WARNING:root:DRYRUN: would run 'juju relate telegraf:juju-info rabbitmq:juju-info'",
                 'WARNING:root:ubuntu-advantage is deployed in containers - skipping ubuntu-advantage',
             ]
-            utils.relations.process_charms_to_relate(args, ["juju", "relate"], applications, None)
+            utils.relations.process_charms_to_relate(args, ["juju", "relate"], applications, {})
             self.assertEqual(_logs.output, expected_logs)
         # Test already related.
         _already_related.return_value = True
@@ -62,7 +72,7 @@ class TestUtilsRelations(unittest.TestCase):
                 'WARNING:root:telegraf is already related to rabbitmq',
                 'WARNING:root:ubuntu-advantage is already related to rabbitmq',
             ]
-            utils.relations.process_charms_to_relate(args, ["juju", "relate"], applications, None)
+            utils.relations.process_charms_to_relate(args, ["juju", "relate"], applications, {})
             self.assertEqual(_logs.output, expected_logs)
         # Test hulk-smashed.
         _deployed_to_containers.return_value = False
@@ -78,5 +88,5 @@ class TestUtilsRelations(unittest.TestCase):
                 "primary",
                 "WARNING:root:DRYRUN: would run 'juju relate ubuntu-advantage:juju-info mysql:juju-info'",
             ]
-            utils.relations.process_charms_to_relate(args, ["juju", "relate"], applications, None)
+            utils.relations.process_charms_to_relate(args, ["juju", "relate"], applications, {})
             self.assertEqual(_logs.output, expected_logs)
diff --git a/utils/relations.py b/utils/relations.py
index 7b00683..fc7b6aa 100644
--- a/utils/relations.py
+++ b/utils/relations.py
@@ -14,15 +14,17 @@ def parse_charms_to_relate():
     for item in charm_config:
         charm_relation = {"source_application": item}
         if "install-everywhere" in charm_config[item] and charm_config[item]["install-everywhere"]:
-            if "source-relation" in charm_config[item]:
-                charm_relation["source_relation"] = charm_config[item]["source-relation"]
+            # Translate keys from yaml to python-friendly format.
+            for key in ["source-relation", "optional"]:
+                if key in charm_config[item]:
+                    charm_relation[key.replace("-", "_")] = charm_config[item][key]
             charms_to_relate.append(charm_relation)
         if "relations" in charm_config[item]:
             for relation in charm_config[item]["relations"]:
                 charm_relation = {"source_application": item}
                 # Translate keys from yaml to python-friendly format.
                 for key in ["source-relation", "target-application", "target-relation", "condition-check-cmd",
-                            "condition-check-output-split-fail-include"]:
+                            "condition-check-output-split-fail-include", "optional"]:
                     if key in relation:
                         charm_relation[key.replace("-", "_")] = relation[key]
                 charms_to_relate.append(charm_relation)
@@ -54,6 +56,9 @@ def process_charms_to_relate(args, command, applications, applications_status):
             relation_data.update(charm_to_relate)
             if relation_data["target_application"] and relation_data["target_application"] != application:
                 continue
+            # Skip optional charms if they are not deployed.
+            if relation_data.get("optional") and relation_data["source_application"] not in applications_status:
+                continue
             # If this application is a primary charm and is on a machine that
             # has another primary charm deployed to it, don't install telegraf.
             if relation_data["source_application"] == "telegraf" and utils.juju.is_primary_hulk_smashed(