Repository: osbuild/osbuild
Pull request #1900 (2024-10-10 16:25:02)

schema: `metadata` at the top level
A new metadata key at the top level which contains a generators key which is a list of all generators involved in the creation of the manifest.

Tools that generate manifests are, for example: [otk](https://github.com/osbuild/otk), [osbuild-composer](https://github.com/osbuild/osbuild-composer) (through [images](https://github.com/osbuild/images)).

supakeen: Now has a test; one of the test manifests includes the `meta` key at the top level.
achilleas-k: > The obviuos one would be:
> 
> * generator, generator-version
> 
> What else would we put here?

```json
{
  "generator": {
    "name": "osbuild/images",
    "version": "v0.90",
    "timestamp": "1728905899"
  }
}
```

supakeen: That'd be one example; another example could be:

- customizations used to generate the image
- image type
- pre-depsolve package set
- an original omnifest
- the sbom related to everything inside the image (though we might want to move that under a structured field)
supakeen: @mvo5 my idea would be to have `meta` be there and not depend on it (since it doesn't have a  schema), start there with conventions to see if we like things and afterwards propose things for inclusion at the top level schema.
achilleas-k: > Thanks for listing some of the planned content. I must say this makes me very uneasy. We have this very nice osbuild json format which is versioned and validated. Now we are adding a "meta" tree that can contain anything (and some of the example mean a lot of extra data).
> 
> I wonder if instead of putting this all into the (so far) well defined osbuild manifest we should create a new container format that includes this additional typed data (like the otk file) and in addition the osbuild manifest. This could be as simple as a tar file.
> 
> I like "meta" but I dislike that it's not typed, maybe we can find a compromise, add some meta keys (like generator.{name,version,timestamp}) into our schema and put the rest into this new container?

Thanks for this comment, it nicely expresses my feelings about the change.

I'm finding it hard to explain exactly what makes me uneasy about this.  My thinking keeps cycling through:
- This is fine because it doesn't affect the functionality or the identity of the manifest.
- If it doesn't affect the functionality or identify of the manifest, then what is it for and why does it need to be part of the manifest?
- It's metadata tied to a manifest that could be used by other tools to describe the origin or purpose of the manifest (or anything).
- If it's meant for other tools, maybe those tools should have their own manifest-adjacent, external metadata for its manifests.
- But keeping them together means the metadata is more tightly bound to the manifest itself.

> start there with conventions to see if we like things and afterwards propose things for inclusion at the top level schema.

Starting wide open and locking down never works, in my experience.  The opposite does though.

I think we should consider the purpose and start with at least some restrictions.  One thing that would make sense to me is to at least restrict the top level type to strings, and maybe even some of the structure of each element beneath that.  So, for example, I would enforce something like:
```json
{
  "meta": {
    "title": "Arbitrary meta information keyed by project identifier",
    "type": "object",
    "additionalProperties": false,
    "patternProperties": {
      "^.*$": {
        "type": "object",
        "additionalProperties": false,
        "properties": {
          "description": {
            "type": "string"
          },
          "metadata": {
            "type": "object",
            "additionalProperties": true
          }
        }
      }
    }
  }
}
```

(This is just a quick recommendation of what I'm considering and I don't expect it to be taken without discussion or modification)

My thinking here is that it enforces, at least, that top-level keys under `"meta"` are strings and suggests that each tool should add its name as the top-level key and a description below that, alongside the metadata it wants to store.  So we could have something like:
```json
{
  "meta": {
    "osbuild-composer": {
      "description": "Manifest created by osbuild-composer v101 (https://github.com/osbuild/osbuild-composer)",
      "metadata": {
        "version": "101",
        "created": "2024-10-17 14:05",
        "emoji of the day": "💥"
      }
    },
    "osbuild-depsolve-dnf": {
      "description": "Package versions and dependencies resolved using osbuild-depsolve-dnf from osbuild v120 (https://github.com/osbuild/osbuild)",
      "metadata": {
        "version": "120",
        "package selection": {
          "build": [ "..." ],
          "os": [ "..." ]
        }
      }
    }
  }
}
```

supakeen: @mvo5 / @achilleas-k, since we/I only have a concrete example for the generator I propose the following:

```
metadata:
  generator:
    [strkey]:
      version: string
```

This would allow for multiple generators to save their information if that's something we want in the future. If we go directly with `name`/`version` under generator we'd preclude that.

Is that OK?
mvo5: > @mvo5 / @achilleas-k, since we/I only have a concrete example for the generator I propose the following:
> 
> ```
> metadata:  # instead of meta
>   generator:
>     [strkey]:  # so we can store information for multiple if that ever applies
>       version: string
> ```
> 
> This would allow for multiple generators to save their information if that's something we want in the future. If we go directly with `name`/`version` under generator we'd preclude that.
> 
> Is that OK?

That works for me, fwiw, I don't have an opinion on "meta" vs "metadata" (the later is a bit more specific which is probably nicer). I am also fine with version/timestamp(or created)/vcs_revision as potential fields as those seem to have obvious use-cases.
supakeen: Updated and adjusted; there's now a new metadata key at the top level which contains a generators key which is a list of all generators involved in the creation of the manifest.
supakeen: > This is very nice now, one tiny suggesiton inline and tests are failing(I have not looked in detail why though)

Because I made a booboo that I've fixed :)
supakeen: > Thanks! This looks fine now, Sorry that I drag this on further :( But I had one interesting observation while reviewing.
> 
> I added the following snippet to the existing format test:
> 
> ```python
> diff --git a/test/mod/test_fmt_v2.py b/test/mod/test_fmt_v2.py
> index 65f7d3ba..f1014850 100644
> --- a/test/mod/test_fmt_v2.py
> +++ b/test/mod/test_fmt_v2.py
> @@ -12,6 +12,14 @@ import osbuild.meta
>  
>  BASIC_PIPELINE = {
>      "version": "2",
> +    "metadata": {
> +        "generators": [
> +            {
> +                "name": "handcrafted",
> +                "version": "NaN",
> +            }
> +        ],
> +    },
>      "sources": {
>          "org.osbuild.curl": {
>              "items": {
> ```
> 
> and when doing this I saw that the `test_describe()` fails because the manifest is not reproduced with "describe()", the "metadata" object is omited - it seems osbuild/format/v2.py would need a new "def describe metadata" (or something like this).
> 
> It's an interesting question if `--inspect` should include the metadata or not (maybe @achilleas-k has an opinions here?). I personally have not used `--inspect` myself. My gut feeling is that we should reproduce the manifest as faithfully as possible and include the metadata but I can see an argument against this. (But I can see the other side too) If we not include metadata in describe(), it is probably good to add a comment in osbuild/format/v2.py about this, something like `# "metadata" object not included as it is not required for the functionality of osbuild`.

This is a really good find. It's better to start with exactly reproducing the manifest. I'll update this PR to include the change you made in `test_fmt_v2.py` and adjust the describe-parts.
supakeen: Ok, I've re-ordered the imports by hand (nothing else changed) to what they should be but the above is worrisome.

Patch:
diff --git a/osbuild/formats/v2.py b/osbuild/formats/v2.py
index 775be12c..0fd5bf73 100644
--- a/osbuild/formats/v2.py
+++ b/osbuild/formats/v2.py
@@ -166,6 +166,9 @@ def describe(manifest: Manifest, *, with_id=False) -> Dict:
         "pipelines": pipelines
     }
 
+    if manifest.metadata:
+        description["metadata"] = manifest.metadata
+
     if sources:
         description["sources"] = sources
 
@@ -355,10 +358,15 @@ def load(description: Dict, index: Index) -> Manifest:
 
     sources = description.get("sources", {})
     pipelines = description.get("pipelines", [])
+    metadata = description.get("metadata", {})
 
     manifest = Manifest()
     source_refs = set()
 
+    # metadata
+    for key, value in metadata.items():
+        manifest.add_metadata(key, value)
+
     # load the sources
     for name, desc in sources.items():
         info = index.get_module_info("Source", name)
diff --git a/osbuild/pipeline.py b/osbuild/pipeline.py
index 84013740..c955033b 100644
--- a/osbuild/pipeline.py
+++ b/osbuild/pipeline.py
@@ -4,7 +4,7 @@ import hashlib
 import json
 import os
 from fnmatch import fnmatch
-from typing import Dict, Generator, Iterable, Iterator, List, Optional
+from typing import Any, Dict, Generator, Iterable, Iterator, List, Optional
 
 from . import buildroot, host, objectstore, remoteloop
 from .api import API
@@ -388,9 +388,13 @@ class Manifest:
     """Representation of a pipeline and its sources"""
 
     def __init__(self):
+        self.metadata = {}
         self.pipelines = collections.OrderedDict()
         self.sources = []
 
+    def add_metadata(self, name: str, data: Dict[str, Any]) -> None:
+        self.metadata[name] = data
+
     def add_pipeline(
         self,
         name: str,
diff --git a/schemas/osbuild2.json b/schemas/osbuild2.json
index faf7804d..00d4e2ed 100644
--- a/schemas/osbuild2.json
+++ b/schemas/osbuild2.json
@@ -19,6 +19,9 @@
       "enum": [
         "2"
       ]
+    },
+    "metadata": {
+      "$ref": "#/definitions/metadata"
     }
   },
   "definitions": {
@@ -83,6 +86,31 @@
         }
       }
     },
+    "metadata": {
+      "title": "Metadata information for a manifest",
+      "type": "object",
+      "additionalProperties": false,
+      "properties": {
+        "generators": {
+          "type": "array",
+          "items": {
+            "type": "object",
+            "additionalProperties": false,
+            "required": [
+              "name"
+            ],
+            "properties": {
+              "name": {
+                "type": "string"
+              },
+              "version": {
+                "type": "string"
+              }
+            }
+          }
+        }
+      }
+    },
     "mounts": {
       "title": "Collection of mount points for a stage",
       "type": "array",


Test patch:
diff --git a/test/data/manifests/fedora-boot.json b/test/data/manifests/fedora-boot.json
index 96b3403c..149327a6 100644
--- a/test/data/manifests/fedora-boot.json
+++ b/test/data/manifests/fedora-boot.json
@@ -1,5 +1,13 @@
 {
   "version": "2",
+  "metadata": {
+    "generators": [
+      {
+        "name": "osbuild-mpp",
+        "version": "unknown"
+      }
+    ]
+  },
   "pipelines": [
     {
       "runner": "org.osbuild.fedora39",
diff --git a/test/data/manifests/fedora-boot.mpp.yaml b/test/data/manifests/fedora-boot.mpp.yaml
index 9abc62c0..f8e5dda1 100644
--- a/test/data/manifests/fedora-boot.mpp.yaml
+++ b/test/data/manifests/fedora-boot.mpp.yaml
@@ -2,6 +2,10 @@
 # Noticed the org.osbuild.test that was added.
 
 version: '2'
+metadata:
+  generators:
+    - name: osbuild-mpp
+      version: unknown
 pipelines:
   - mpp-import-pipelines:
       path: fedora-vars.ipp.yaml
diff --git a/test/mod/test_fmt_v2.py b/test/mod/test_fmt_v2.py
index 65f7d3ba..6b570835 100644
--- a/test/mod/test_fmt_v2.py
+++ b/test/mod/test_fmt_v2.py
@@ -12,6 +12,14 @@ import osbuild.meta
 
 BASIC_PIPELINE = {
     "version": "2",
+    "metadata": {
+        "generators": [
+            {
+                "name": "handcrafted",
+                "version": "NaN",
+            },
+        ],
+    },
     "sources": {
         "org.osbuild.curl": {
             "items": {