Merge ~jslarraz/review-tools:schema-split-definitions into review-tools:master

[Jorge Sancho Larraz]

rt/schemas,bin/store-query: move architectures list to a new definitions.json

[Jorge Sancho Larraz (jslarraz)]

Stripping some value definitions from the snap.yaml schema will allow us to reuse them in the project (architectures, types, etc. in snap.yaml and snapcraft.yaml) or among projects (title, summary, etc. for snaps, rocks, etc.)

Reference to documents in local filesystem does not work on focal. This approach would require to move to a newer release or to place schemas in a publicly accessible location under a stable url pattern or ideally both.

Thoughts about it?

[Alex Murray (alexmurray)]

Perhaps I am missing something, but is it possible to load the schema and use the values directly from it rather than storing them in a separate file?

[Jorge Sancho Larraz (jslarraz)]

That's certainly possible as done in https://git.launchpad.net/~jslarraz/review-tools/commit/?id=22827dce8143d4e685f992c049a2935a744285ba.

However, it brings no re usability benefit. For example, looking at this MR https://code.launchpad.net/~jslarraz/review-tools/+git/review-tools/+merge/466454, ideally at some point we would like to validate that the declaration is properly formatted using a declaration schema rather than in code. In such a situation, I think it will be better (for the maintainability) to define the snap types in a separate file that can be imported by both snap.json and declaration.json rather than two separate list.

[Alex Murray (alexmurray)]

Ah right - so since we want to have separate JSON schemas for validating the snap declaration and for validating the snap yaml, we want to then share common type definitions between the two - this was the context I was missing.

So I am not against this change then but as we don't yet have a snap declaration schema, I think it seems a bit like premature optimisation to split out the types into a separate json schema at this time - however, since you are leading this work I am happy to go with your preferred approach. Feel free to go forward with this separate definitions.json approach :)

LGTM!

review: Approve

Unmerged commits

46940fd... by Jorge Sancho Larraz on 2024-05-23

rt/schemas,bin/store-query: move architectures list to a new definitions.json

	test:0 (build)
	coverage:0 (build)

1 → 2 of 2 results

First • Previous • Next • Last

diff --git a/bin/store-query b/bin/store-query
index e6b22b8..e1938c5 100755
--- a/bin/store-query
+++ b/bin/store-query
@@ -220,20 +220,14 @@ def _get_snap_yaml(snap_name, channel=None, arch=None):
     headers = {"Snap-Device-Series": snap_device_series}
     res = _fetch(url, headers)
 
+    with open("reviewtools/schemas/definitions.json") as fd:
+        definitions = json.loads(fd.read())
+
     risk = "stable"
     track = "latest"
     if arch is None:
         arch = "amd64"
-    if arch not in [
-        "amd64",
-        "arm64",
-        "armhf",
-        "i386",
-        "powerpc",
-        "ppc64el",
-        "riscv64",
-        "s390x",
-    ]:
+    if arch not in definitions["$defs"]["architecture"]["enum"]:
         raise ValueError("Unsupported arch '%s'" % arch)
 
     if channel is not None:
diff --git a/reviewtools/schemas/definitions.json b/reviewtools/schemas/definitions.json
 new file mode 100644
index 0000000..ae8b4a8
--- /dev/null
+++ b/reviewtools/schemas/definitions.json
@@ -0,0 +1,9 @@
+{
+  "description": "snap related definitions",
+  "$defs": {
+    "architecture": {
+      "type": "string",
+      "enum": ["amd64", "arm64", "armhf", "i386", "powerpc", "ppc64el", "riscv64", "s390x"]
+    }
+  }
+}
 \ No newline at end of file
diff --git a/reviewtools/schemas/snap.json b/reviewtools/schemas/snap.json
index 8d9c6bd..15809b5 100644
--- a/reviewtools/schemas/snap.json
+++ b/reviewtools/schemas/snap.json
@@ -21,8 +21,15 @@
       "type": "array",
       "uniqueItems": true,
       "items": {
-        "type": "string",
-        "enum": ["all", "amd64", "arm64", "armhf", "i386", "powerpc", "ppc64el", "riscv64", "s390x"]
+        "anyOf": [
+          {
+            "type": "string",
+            "enum": ["all"]
+          },
+          {
+            "$ref": "definitions.json#/$defs/architecture"
+          }
+        ]
       },
       "minItems": 1,
       "default": ["all"]
diff --git a/reviewtools/tests/schemas/test_schema_snap.py b/reviewtools/tests/schemas/test_schema_snap.py
index ed4853d..c7ff6ab 100644
--- a/reviewtools/tests/schemas/test_schema_snap.py
+++ b/reviewtools/tests/schemas/test_schema_snap.py
@@ -144,19 +144,22 @@ class TestSchemaSnap(TestSchemaBase):
             # test_check_architectures_single_ppc64el
             (["ppc64el"], None),
             # test_check_architectures_single_nonexistent
-            (["nonexistent"], "'nonexistent' is not one of "),
+            (
+                ["nonexistent"],
+                "'nonexistent' is not valid under any of the given schemas",
+            ),
             # test_check_snappy_valid_arch_multi
             (["amd64", "armhf"], None),
             # test_check_snappy_valid_arch_multi2
             (["armhf", "arm64", "i386"], None),
             # test_check_architectures_bad_entry
-            ([{}], "{} is not of type 'string'"),
+            ([{}], "{} is not valid under any of the given schemas"),
             # ### integer
             (2, "{value} is not of type 'array'"),
             # ### empty TODO: snapcraft does not allow empty lists, snapd does
             ([], "{value} is too short"),
             # ### empty value
-            ([""], "'' is not one of "),
+            ([""], "'' is not valid under any of the given schemas"),
             # ### duplicated value TODO: snapcraft does not duplicated values, snapd does
             (["amd64", "amd64"], "{value} has non-unique elements"),
         ]: