Merge ~jslarraz/review-tools:initial-schema-support into review-tools:master

[Jorge Sancho Larraz]

Initial schema support

Set up the framework to verify snap.yaml via schema. Attributes will be included in the schema iteratively to make PR reasonable to review

[Jorge Sancho Larraz (jslarraz)]

review: Needs Fixing

Unmerged commits

e23df21... by Jorge Sancho Larraz on 2024-05-07

Initial schema support

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

1 → 2 of 2 results

First • Previous • Next • Last

diff --git a/reviewtools/schemas/snap.json b/reviewtools/schemas/snap.json
new file mode 100644
index 0000000..3c239c4
--- /dev/null
+++ b/reviewtools/schemas/snap.json
@@ -0,0 +1,14 @@
+{
+  "description": "snap.yaml",
+  "type": "object",
+  "properties": {
+    "name": {
+      "description": "The identifying name of the snap.",
+      "type": "string",
+      "pattern": "^(?:[a-z0-9]|(?<=[a-z0-9])-)*[a-z](?:[a-z0-9]|-(?=[a-z0-9]))*$",
+      "minLength": 2,
+      "maxLength": 40
+    }
+  },
+  "required": ["name"]
+}
diff --git a/reviewtools/sr_lint.py b/reviewtools/sr_lint.py
index c94340a..e1175e8 100644
--- a/reviewtools/sr_lint.py
+++ b/reviewtools/sr_lint.py
@@ -18,8 +18,6 @@ from __future__ import print_function
 from reviewtools.sr_common import SnapReview
 from reviewtools.common import (
     find_external_symlinks,
-    STORE_PKGNAME_SNAPV2_MAXLEN,
-    STORE_PKGNAME_SNAPV2_MINLEN,
 )
 from reviewtools.overrides import (
     redflagged_snap_types_overrides,
@@ -33,6 +31,8 @@ import os
 import re
 import shlex
 import unicodedata
+import jsonschema
+import json
 
 
 class SnapReviewLint(SnapReview):
@@ -77,6 +77,26 @@ class SnapReviewLint(SnapReview):
             "test-snapd-refresh-control-base",
         ]
 
+    def check_schema(self):
+        """Check snap.yaml against the schema"""
+        t = "info"
+        n = self._get_check_name("snap_schema")
+        s = "OK"
+        manual_review = False
+
+        with open("reviewtools/schemas/snap.json") as fd:
+            schema = json.loads(fd.read())
+
+        try:
+            jsonschema.validate(self.snap_yaml, schema)
+        except jsonschema.ValidationError as e:
+            t = "error"
+            s = "Error found in snap.yaml while validating %s: %s" % (
+                e.json_path,
+                e.message,
+            )
+            self._add_result(t, n, s, manual_review=manual_review)
+
     def check_architectures(self):
         """Check architectures in snap.yaml is valid"""
         t = "info"
@@ -175,39 +195,6 @@ class SnapReviewLint(SnapReview):
         # snap/validate.go
         self._check_description_summary_title("title", 40)
 
-    def check_name(self):
-        """Check package name"""
-        t = "info"
-        n = self._get_check_name("name_valid")
-        s = "OK"
-        if "name" not in self.snap_yaml:
-            t = "error"
-            s = "could not find 'name' in yaml"
-        elif not isinstance(self.snap_yaml["name"], str):
-            t = "error"
-            s = "malformed 'name': %s (not a str)" % (self.snap_yaml["name"])
-        elif len(self.snap_yaml["name"]) > STORE_PKGNAME_SNAPV2_MAXLEN:
-            t = "error"
-            s = (
-                "malformed 'name': '%s' " % self.snap_yaml["name"]
-                + "(length '%d' " % len(self.snap_yaml["name"])
-                + "exceeds store limit '%d')" % STORE_PKGNAME_SNAPV2_MAXLEN
-            )
-        elif len(self.snap_yaml["name"]) < STORE_PKGNAME_SNAPV2_MINLEN:
-            t = "error"
-            s = (
-                "malformed 'name': '%s' " % self.snap_yaml["name"]
-                + "(length '%d' " % len(self.snap_yaml["name"])
-                + "below store limit '%d')" % STORE_PKGNAME_SNAPV2_MINLEN
-            )
-        elif not self._verify_pkgname(self.snap_yaml["name"]):
-            t = "error"
-            s = (
-                "malformed 'name': '%s' " % self.snap_yaml["name"]
-                + "(may be only lower case, digits and hyphens)"
-            )
-        self._add_result(t, n, s)
-
     def check_type(self):
         """Check type"""
         t = "info"
diff --git a/reviewtools/tests/schemas/test_schema_against_store.py b/reviewtools/tests/schemas/test_schema_against_store.py
new file mode 100644
index 0000000..16259e2
--- /dev/null
+++ b/reviewtools/tests/schemas/test_schema_against_store.py
@@ -0,0 +1,76 @@
+from subprocess import run
+import json
+import yaml
+import os
+import sys
+import jsonschema
+import unittest
+
+SNAP_SCHEMA = "reviewtools/schemas/snap.json"
+SNAP_YAML_CACHE = os.path.expanduser("~/.cache/review-tools")
+
+
[e-mail](
+    "TEST_SCHEMA_AGAINST_STORE" in os.environ, "not intended to be run by lpci"
+)
+def test_store():
+    # Create cache directory if it does not exist
+    try:
+        os.mkdir(
+            SNAP_YAML_CACHE,
+        )
+    except OSError:
+        pass
+
+    # Load schema
+    with open(SNAP_SCHEMA) as fd:
+        snap_schema = json.loads(fd.read())
+
+    # Get list of snaps in the store
+    with open("/var/cache/snapd/names", "r") as fd:
+        snaps = fd.readlines()
+
+    rc = 0
+    for snap in snaps:
+        snap = snap.strip("\n ")
+
+        # Load the snap.yaml if cached, otherwise download it from the store
+        snap_file = os.path.join(SNAP_YAML_CACHE, snap + "_yaml.json")
+        if os.path.exists(snap_file):
+            with open(snap_file) as fd:
+                snap_yaml = json.loads(fd.read())
+        else:
+            try:
+                p = run(
+                    ["review-tools.store-query", "--snap-yaml", snap],
+                    capture_output=True,
+                )
+                snap_yaml = yaml.safe_load(p.stdout.decode())
+            except Exception as e:
+                print(
+                    "Error occurred while getting snap.yaml for %s: %s" % (snap, str(e))
+                )
+                continue
+
+            with open(snap_file, "w") as fd:
+                fd.write(json.dumps(snap_yaml))
+
+        # There are some snaps that has no snap.yaml. Skip them for now
+        if snap_yaml is None:
+            continue
+
+        # Validate the snap.yaml against the schema
+        try:
+            jsonschema.validate(snap_yaml, snap_schema)
+        except jsonschema.ValidationError as e:
+            print(
+                "Error to validate %s for snap %s: %s" % (e.json_path, snap, e.message)
+            )
+            rc = 1
+
+    sys.exit(rc)
+
+
+if __name__ == "__main__":
+    if os.environ.get("TEST_SCHEMA_AGAINST_STORE"):
+        test_store()
diff --git a/reviewtools/tests/schemas/test_schema_base.py b/reviewtools/tests/schemas/test_schema_base.py
new file mode 100644
index 0000000..b41f5dd
--- /dev/null
+++ b/reviewtools/tests/schemas/test_schema_base.py
@@ -0,0 +1,44 @@
+import json
+import jsonschema
+import copy
+import unittest
+
+
+class TestSchemaBase(unittest.TestCase):
+    yaml = {}
+    schema_file = "reviewtools/schemas/****.json"
+
+    def setUp(self):
+        # Load the relevant schema (defined in subclass)
+        with open(self.schema_file) as fd:
+            self.schema = json.loads(fd.read())
+
+        # Validate base yaml against the schema
+        error = self._validate(self.yaml, self.schema)
+        self.assertEqual(None, error)
+
+    def _validate(self, yaml, schema):
+        # Validate yaml against the schema
+        try:
+            jsonschema.validate(yaml, schema)
+            return None
+        except jsonschema.ValidationError as e:
+            return e.message
+
+    def _test_value(self, key, value, expected_error):
+        # Prepare yaml, make a copy so we use the pristine yaml on each subtest
+        yaml = copy.deepcopy(self.yaml)
+        if value is None:
+            if key in yaml:
+                del yaml[key]
+        else:
+            yaml[key] = value
+
+        # Validate the schema
+        error = self._validate(yaml, self.schema)
+
+        # Compare the errors against the expected ones
+        if expected_error is None:
+            self.assertIsNone(error)
+        else:
+            self.assertIn(expected_error, error)
diff --git a/reviewtools/tests/schemas/test_schema_snap.py b/reviewtools/tests/schemas/test_schema_snap.py
new file mode 100644
index 0000000..315b278
--- /dev/null
+++ b/reviewtools/tests/schemas/test_schema_snap.py
@@ -0,0 +1,67 @@
+from reviewtools.tests.schemas.test_schema_base import TestSchemaBase
+
+
+class TestSchemaSnap(TestSchemaBase):
+
+    schema_file = "reviewtools/schemas/snap.json"
+
+    def setUp(self):
+        self.yaml = {
+            "name": "foo",
+            "version": "0.1",
+            "type": "app",
+            "description": "Test description",
+            "summary": "Test summary",
+        }
+        super(TestSchemaSnap, self).setUp()
+
+    def test_name(self):
+        for value, error in [
+            # test_check_name_toplevel
+            ("foo", None),
+            # test_check_name_toplevel_startswith_number
+            ("01game", None),
+            # test_check_name_toplevel_maxlen_for_store
+            ("a" * 40, None),
+            # test_check_name_toplevel_minlen_for_store
+            ("a" * 2, None),
+            # test_check_name_toplevel_too_long_for_store
+            ("a" * 41, "'{value}' is too long"),
+            # test_check_name_toplevel_too_short_for_store
+            ("a", "'{value}' is too short"),
+            # test_check_name_toplevel_efficient
+            ("u-94903713687486543234157734673284536758", None),
+            # test_check_name_flat
+            ("foo.bar", "'{value}' does not match "),
+            # test_check_name_reverse_domain
+            ("com.ubuntu.develeper.baz.foo", "'{value}' does not match "),
+            # test_check_name_bad - ?
+            ("foo?bar", "'{value}' does not match "),
+            # test_check_name_bad1 - /
+            ("foo/bar", "'{value}' does not match "),
+            # test_check_name_bad2 - empty
+            ("", "'{value}' does not match "),
+            # test_check_name_bad3 - list
+            ([], "{value} is not of type 'string'"),
+            # test_check_name_bad4 - dict
+            ({}, "{value} is not of type 'string'"),
+            # test_check_name_bad5 - --
+            ("foo--bar", "'{value}' does not match "),
+            # test_check_name_bad6 - endswith -
+            ("foo-bar-", "'{value}' does not match "),
+            # test_check_name_bad6b - startswith -
+            ("-foo-bar", "'{value}' does not match "),
+            # test_check_name_bad7
+            ("foo-Bar", "'{value}' does not match "),
+            # test_check_name_bad8 - cap
+            ("01", "'{value}' does not match "),
+            # test_check_name_bad9 -  all numbers and dashes
+            ("0-1", "'{value}' does not match "),
+            # test_check_name_missing
+            (None, "'name' is a required property"),
+            # ### integer
+            (2, "{value} is not of type 'string'"),
+        ]:
+            with self.subTest(value=value):
+                error = error.replace("{value}", str(value)) if error else error
+                self._test_value("name", value, error)
diff --git a/reviewtools/tests/test_sr_lint.py b/reviewtools/tests/test_sr_lint.py
index 46f141c..d720017 100644
--- a/reviewtools/tests/test_sr_lint.py
+++ b/reviewtools/tests/test_sr_lint.py
@@ -144,215 +144,6 @@ class TestSnapReviewLint(sr_tests.TestSnapReview):
             sum += len(c.review_report[i])
         self.assertTrue(sum != 0)
 
-    def test_check_name_toplevel(self):
-        """Test check_name - toplevel"""
-        self.set_test_snap_yaml("name", "foo")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": 1, "warn": 0, "error": 0}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_toplevel_startswith_number(self):
-        """Test check_name - toplevel starts with number"""
-        self.set_test_snap_yaml("name", "01game")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": 1, "warn": 0, "error": 0}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_toplevel_maxlen_for_store(self):
-        """Test check_name - toplevel maxlen for store"""
-        self.set_test_snap_yaml("name", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa40chars")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": 1, "warn": 0, "error": 0}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_toplevel_minlen_for_store(self):
-        """Test check_name - toplevel minlen for store"""
-        self.set_test_snap_yaml("name", "aa")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": 1, "warn": 0, "error": 0}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_toplevel_too_long_for_store(self):
-        """Test check_name - toplevel too long for store"""
-        self.set_test_snap_yaml("name", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa41chars")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
-        expected = dict()
-        expected["error"] = dict()
-        expected["warn"] = dict()
-        expected["info"] = dict()
-        name = "lint-snap-v2:name_valid"
-        expected["error"][name] = {
-            "text": "malformed 'name': 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa41chars' (length '41' exceeds store limit '40')"
-        }
-        self.check_results(r, expected=expected)
-
-    def test_check_name_toplevel_too_short_for_store(self):
-        """Test check_name - toplevel too short for store"""
-        self.set_test_snap_yaml("name", "a")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
-        expected = dict()
-        expected["error"] = dict()
-        expected["warn"] = dict()
-        expected["info"] = dict()
-        name = "lint-snap-v2:name_valid"
-        expected["error"][name] = {
-            "text": "malformed 'name': 'a' (length '1' below store limit '2')"
-        }
-        self.check_results(r, expected=expected)
-
-    def test_check_name_toplevel_efficient(self):
-        """Test check_name - toplevel is efficient"""
-        self.set_test_snap_yaml("name", "u-94903713687486543234157734673284536758")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": 1, "warn": 0, "error": 0}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_flat(self):
-        """Test check_name - obsoleted flat"""
-        self.set_test_snap_yaml("name", "foo.bar")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_reverse_domain(self):
-        """Test check_name - obsoleted reverse domain"""
-        self.set_test_snap_yaml("name", "com.ubuntu.develeper.baz.foo")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_bad(self):
-        """Test check_name - bad - ?"""
-        self.set_test_snap_yaml("name", "foo?bar")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_bad1(self):
-        """Test check_name - bad - /"""
-        self.set_test_snap_yaml("name", "foo/bar")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_bad2(self):
-        """Test check_name - empty"""
-        self.set_test_snap_yaml("name", "")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_bad3(self):
-        """Test check_name - list"""
-        self.set_test_snap_yaml("name", [])
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_bad4(self):
-        """Test check_name - dict"""
-        self.set_test_snap_yaml("name", {})
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_bad5(self):
-        """Test check_name - bad - --"""
-        self.set_test_snap_yaml("name", "foo--bar")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_bad6(self):
-        """Test check_name - bad - endswith -"""
-        self.set_test_snap_yaml("name", "foo-bar-")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_bad6b(self):
-        """Test check_name - bad - startswith -"""
-        self.set_test_snap_yaml("name", "-foo-bar")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_bad7(self):
-        """Test check_name - bad - cap"""
-        self.set_test_snap_yaml("name", "foo-Bar")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_bad8(self):
-        """Test check_name - bad - all numbers"""
-        self.set_test_snap_yaml("name", "01")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_bad9(self):
-        """Test check_name - bad - all numbers and dashes"""
-        self.set_test_snap_yaml("name", "0-1")
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
-    def test_check_name_missing(self):
-        """Test check_name - missing"""
-        self.set_test_snap_yaml("name", None)
-        c = SnapReviewLint(self.test_name)
-        c.check_name()
-        r = c.review_report
-        expected_counts = {"info": None, "warn": 0, "error": 1}
-        self.check_results(r, expected_counts)
-
     def test_check_version(self):
         """Test check_version"""
         self.set_test_snap_yaml("version", 1)