Repository: osbuild/osbuild
Pull request #1667 (2024-03-18 14:27:16)

sources: transform() is only used in the curl sources, remove  from ABC and rename
The curl source is the only source that uses "transform". And here the name is very generic but in fact we only do a single thing:
we add secrets for subscriptions for for mtls to the download.

So rename to make it clear what this is all about.


Split out from https://github.com/osbuild/osbuild/pull/1573 as it's unrelated and will make reviews for the other simpler.


Patch:
diff --git a/osbuild/sources.py b/osbuild/sources.py
index 491b986a..a3f532e4 100644
--- a/osbuild/sources.py
+++ b/osbuild/sources.py
@@ -4,7 +4,7 @@ import hashlib
 import json
 import os
 import tempfile
-from typing import ClassVar, Dict, Tuple
+from typing import ClassVar, Dict
 
 from . import host
 from .objectstore import ObjectStore
@@ -105,11 +105,6 @@ class SourceService(host.Service):
         """Returns True if the item to download is in cache. """
         return os.path.isfile(f"{self.cache}/{checksum}")
 
-    # pylint: disable=[no-self-use]
-    def transform(self, checksum, desc) -> Tuple:
-        """Modify the input data before downloading. By default only transforms an item object to a Tupple."""
-        return checksum, desc
-
     @staticmethod
     def load_items(fds):
         with os.fdopen(fds.steal(0)) as f:
diff --git a/sources/org.osbuild.containers-storage b/sources/org.osbuild.containers-storage
index 1bfea583..4be3c652 100755
--- a/sources/org.osbuild.containers-storage
+++ b/sources/org.osbuild.containers-storage
@@ -13,7 +13,6 @@ by osbuild itself.
 Buildhost commands used: `skopeo`.
 """
 
-import concurrent.futures
 import hashlib
 import subprocess as sp
 import sys
@@ -66,11 +65,8 @@ class ContainersStorageSource(sources.SourceService):
         self.exists(checksum, desc)
 
     def fetch_all(self, items) -> None:
-        # prepare each item as a (checksum, desc) tuple (where desc=None)
-        transformed = map(lambda i: self.transform(i, None), items)
-        with concurrent.futures.ThreadPoolExecutor(max_workers=self.max_workers) as executor:
-            for _ in executor.map(self.fetch_one, *zip(*transformed)):
-                pass
+        for checksum in items:
+            self.fetch_one(checksum, None)
 
     def exists(self, checksum, _) -> bool:
         image_id = checksum.split(":")[1]
diff --git a/sources/org.osbuild.curl b/sources/org.osbuild.curl
index 86ce1eae..230bb84f 100755
--- a/sources/org.osbuild.curl
+++ b/sources/org.osbuild.curl
@@ -102,29 +102,30 @@ class CurlSource(sources.SourceService):
         super().__init__(*args, **kwargs)
         self.subscriptions = None
 
-    def transform(self, checksum, desc):
-        url = desc
-        if not isinstance(url, dict):
-            url = {"url": url}
-
-        # check if url needs rhsm secrets
-        if url.get("secrets", {}).get("name") == "org.osbuild.rhsm":
+    def amend_secrets(self, checksum, desc_or_url):
+        if not isinstance(desc_or_url, dict):
+            desc = {"url": desc_or_url}
+        else:
+            desc = desc_or_url
+
+        # check if desc needs rhsm secrets
+        if desc.get("secrets", {}).get("name") == "org.osbuild.rhsm":
             # rhsm secrets only need to be retrieved once and can then be reused
             if self.subscriptions is None:
                 self.subscriptions = Subscriptions.from_host_system()
-            url["secrets"] = self.subscriptions.get_secrets(url.get("url"))
-        elif url.get("secrets", {}).get("name") == "org.osbuild.mtls":
+            desc["secrets"] = self.subscriptions.get_secrets(desc.get("desc"))
+        elif desc.get("secrets", {}).get("name") == "org.osbuild.mtls":
             key = os.getenv("OSBUILD_SOURCES_CURL_SSL_CLIENT_KEY")
             cert = os.getenv("OSBUILD_SOURCES_CURL_SSL_CLIENT_CERT")
             if not (key and cert):
                 raise RuntimeError(f"mtls secrets required but key ({key}) or cert ({cert}) not defined")
-            url["secrets"] = {
+            desc["secrets"] = {
                 'ssl_ca_cert': os.getenv("OSBUILD_SOURCES_CURL_SSL_CA_CERT"),
                 'ssl_client_cert': cert,
                 'ssl_client_key': key,
             }
 
-        return checksum, url
+        return checksum, desc
 
     @staticmethod
     def _quote_url(url: str) -> str:
@@ -135,10 +136,10 @@ class CurlSource(sources.SourceService):
 
     def fetch_all(self, items: Dict) -> None:
         filtered = filter(lambda i: not self.exists(i[0], i[1]), items.items())  # discards items already in cache
-        transformed = map(lambda i: self.transform(i[0], i[1]), filtered)  # prepare each item to be downloaded
+        amended = map(lambda i: self.amend_secrets(i[0], i[1]), filtered)
 
         with concurrent.futures.ThreadPoolExecutor(max_workers=self.max_workers) as executor:
-            for _ in executor.map(self.fetch_one, *zip(*transformed)):
+            for _ in executor.map(self.fetch_one, *zip(*amended)):
                 pass
 
     def fetch_one(self, checksum, desc):
diff --git a/sources/org.osbuild.inline b/sources/org.osbuild.inline
index 86313ecd..009c4bbe 100755
--- a/sources/org.osbuild.inline
+++ b/sources/org.osbuild.inline
@@ -59,9 +59,8 @@ class InlineSource(sources.SourceService):
 
     def fetch_all(self, items: Dict) -> None:
         filtered = filter(lambda i: not self.exists(i[0], i[1]), items.items())  # discards items already in cache
-        transformed = map(lambda i: self.transform(i[0], i[1]), filtered)  # prepare each item to be downloaded
 
-        for args in transformed:
+        for args in filtered:
             self.fetch_one(*args)
 
     def fetch_one(self, checksum, desc):
diff --git a/sources/org.osbuild.ostree b/sources/org.osbuild.ostree
index ed590eda..b7bef478 100755
--- a/sources/org.osbuild.ostree
+++ b/sources/org.osbuild.ostree
@@ -91,10 +91,9 @@ class OSTreeSource(sources.SourceService):
 
     def fetch_all(self, items: Dict) -> None:
         filtered = filter(lambda i: not self.exists(i[0], i[1]), items.items())  # discards items already in cache
-        transformed = map(lambda i: self.transform(i[0], i[1]), filtered)  # prepare each item to be downloaded
 
         with concurrent.futures.ThreadPoolExecutor(max_workers=self.max_workers) as executor:
-            for _ in executor.map(self.fetch_one, *zip(*transformed)):
+            for _ in executor.map(self.fetch_one, *zip(*filtered)):
                 pass
 
     def fetch_one(self, checksum, desc):
diff --git a/sources/org.osbuild.skopeo b/sources/org.osbuild.skopeo
index 549cd384..6186d137 100755
--- a/sources/org.osbuild.skopeo
+++ b/sources/org.osbuild.skopeo
@@ -104,10 +104,9 @@ class SkopeoSource(sources.SourceService):
 
     def fetch_all(self, items: Dict) -> None:
         filtered = filter(lambda i: not self.exists(i[0], i[1]), items.items())  # discards items already in cache
-        transformed = map(lambda i: self.transform(i[0], i[1]), filtered)  # prepare each item to be downloaded
 
         with concurrent.futures.ThreadPoolExecutor(max_workers=self.max_workers) as executor:
-            for _ in executor.map(self.fetch_one, *zip(*transformed)):
+            for _ in executor.map(self.fetch_one, *zip(*filtered)):
                 pass
 
     def fetch_one(self, checksum, desc):
diff --git a/sources/org.osbuild.skopeo-index b/sources/org.osbuild.skopeo-index
index 4f056d52..8f23dc69 100755
--- a/sources/org.osbuild.skopeo-index
+++ b/sources/org.osbuild.skopeo-index
@@ -89,10 +89,9 @@ class SkopeoIndexSource(sources.SourceService):
 
     def fetch_all(self, items: Dict) -> None:
         filtered = filter(lambda i: not self.exists(i[0], i[1]), items.items())  # discards items already in cache
-        transformed = map(lambda i: self.transform(i[0], i[1]), filtered)  # prepare each item to be downloaded
 
         with concurrent.futures.ThreadPoolExecutor(max_workers=self.max_workers) as executor:
-            for _ in executor.map(self.fetch_one, *zip(*transformed)):
+            for _ in executor.map(self.fetch_one, *zip(*filtered)):
                 pass
 
     def fetch_one(self, checksum, desc):
diff --git a/stages/org.osbuild.systemd.unit.create b/stages/org.osbuild.systemd.unit.create
index 1662fea5..f562896f 100755
--- a/stages/org.osbuild.systemd.unit.create
+++ b/stages/org.osbuild.systemd.unit.create
@@ -15,6 +15,7 @@ of options:
   - 'Unit' section
     - 'Description' - string
     - 'ConditionPathExists' - string
+    - 'ConditionPathIsDirectory' - string
     - 'DefaultDependencies' - bool
     - 'Requires' - [strings]
     - 'Wants' - [strings]
@@ -90,6 +91,12 @@ SCHEMA = r"""
                             "type": "string"
                         }
                     },
+                    "ConditionPathIsDirectory": {
+                        "type": "array",
+                        "items": {
+                            "type": "string"
+                        }
+                    },
                     "DefaultDependencies": {
                         "type": "boolean"
                     }


Test patch:
diff --git a/sources/test/test_curl_source.py b/sources/test/test_curl_source.py
index 8ac4c8fd..a65a4018 100644
--- a/sources/test/test_curl_source.py
+++ b/sources/test/test_curl_source.py
@@ -36,7 +36,7 @@ def test_curl_source_exists(sources_module):
     assert curl_source.exists(checksum, desc)
 
 
-def test_curl_source_transform(sources_module):
+def test_curl_source_amend_secrets(sources_module):
     sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
     curl_source = sources_module.CurlSource.from_args(["--service-fd", str(sock.fileno())])
     tmpdir = tempfile.TemporaryDirectory()
@@ -58,13 +58,13 @@ def test_curl_source_transform(sources_module):
         cm.callback(cb)
         checksum = "sha256:1234567890123456789012345678901234567890909b14ffb032aa20fa23d9ad6"
         pathlib.Path(os.path.join(tmpdir.name, checksum)).touch()
-        new_desc = curl_source.transform(checksum, desc)
+        new_desc = curl_source.amend_secrets(checksum, desc)
         assert new_desc[1]["secrets"]["ssl_client_key"] == "key"
         assert new_desc[1]["secrets"]["ssl_client_cert"] == "cert"
         assert new_desc[1]["secrets"]["ssl_ca_cert"] is None
 
 
-def test_curl_source_transform_fail(sources_module):
+def test_curl_source_amend_secrets_fail(sources_module):
     sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
     curl_source = sources_module.CurlSource.from_args(["--service-fd", str(sock.fileno())])
     tmpdir = tempfile.TemporaryDirectory()
@@ -78,5 +78,5 @@ def test_curl_source_transform_fail(sources_module):
     checksum = "sha256:1234567890123456789012345678901234567890909b14ffb032aa20fa23d9ad6"
     pathlib.Path(os.path.join(tmpdir.name, checksum)).touch()
     with pytest.raises(RuntimeError) as exc:
-        curl_source.transform(checksum, desc)
+        curl_source.amend_secrets(checksum, desc)
     assert "mtls secrets required" in str(exc)
diff --git a/stages/test/test_systemd_unit_create.py b/stages/test/test_systemd_unit_create.py
index b0f251e0..38adef22 100644
--- a/stages/test/test_systemd_unit_create.py
+++ b/stages/test/test_systemd_unit_create.py
@@ -63,6 +63,9 @@ def test_systemd_unit_create(tmp_path, stage_module, unit_type, unit_path, expec
                 "DefaultDependencies": False,
                 "ConditionPathExists": [
                     "|!/etc/myfile"
+                ],
+                "ConditionPathIsDirectory": [
+                    "|!/etc/mydir"
                 ]
             },
             "Service": {
@@ -94,6 +97,7 @@ def test_systemd_unit_create(tmp_path, stage_module, unit_type, unit_path, expec
     Description=Create directory
     DefaultDependencies=False
     ConditionPathExists=|!/etc/myfile
+    ConditionPathIsDirectory=|!/etc/mydir
 
     [Service]
     Type=oneshot
diff --git a/test/data/stages/systemd.unit.create/b.json b/test/data/stages/systemd.unit.create/b.json
index 8586dae7..c8536607 100644
--- a/test/data/stages/systemd.unit.create/b.json
+++ b/test/data/stages/systemd.unit.create/b.json
@@ -624,6 +624,9 @@
                 "DefaultDependencies": false,
                 "ConditionPathExists": [
                   "|!/etc/myfile"
+                ],
+                "ConditionPathIsDirectory": [
+                  "|!/etc/mydir"
                 ]
               },
               "Service": {
diff --git a/test/data/stages/systemd.unit.create/b.mpp.yaml b/test/data/stages/systemd.unit.create/b.mpp.yaml
index 8586dae7..c8536607 100644
--- a/test/data/stages/systemd.unit.create/b.mpp.yaml
+++ b/test/data/stages/systemd.unit.create/b.mpp.yaml
@@ -624,6 +624,9 @@
                 "DefaultDependencies": false,
                 "ConditionPathExists": [
                   "|!/etc/myfile"
+                ],
+                "ConditionPathIsDirectory": [
+                  "|!/etc/mydir"
                 ]
               },
               "Service": {