Merge lp:~sergiusens/snapcraft/validation into lp:~snappy-dev/snapcraft/core

[Sergio Schvezov]

Initial json schema support

I'm just not sure where to add the tests here, so please advise during the review :-)

[Leo Arias (elopio)]

To test, I think you should encapsulate your additions to the Config.__init__ in a validate method. Then we can pass valid and invalid yamls to it.

Some comments in the diff.

[Zygmunt Krynicki (zyga)]

One more comment below

[Leo Arias (elopio)]

This is great. I left some IMO comments. The only one I feel strong about is splitting the tests in scenarios or subtests.

review: Needs Fixing

[Sergio Schvezov (sergiusens)]

Hey you asked me for subtests a lot and I had them added explained as in the documentation you linked to ;-)

[Leo Arias (elopio)]

how did I not see the subtests??? Sorry about that.
So the only thing that's missing is bumping the dependency to python3 (>= 3.4),

[Leo Arias (elopio)]

review: Approve

[John Lenton (chipaca)]

LGTM. zOMGwtfbbqschemajsonyaml, but LGTM.

review: Approve

[Leo Arias (elopio)]

Once this lands, the guys from erle and spreed have to be notified about the new format, right?

[Snappy Tarmac (snappydevtarmac)]

The attempt to merge lp:~sergiusens/snapcraft/validation into lp:snapcraft failed. Below is the output from the failed tests.

wget -c http://0.0.0.0:39937/test.tar
cp --preserve=all -R zzz /tmp/tmpo5btu74v/parts/copy/install/zzz
cp --preserve=all -R src /tmp/tmpyoazun5w/parts/copy/install/dst
cp --preserve=all -R src /tmp/tmprwukoxqe/parts/copy/install/dir/dst

...........--2015-08-27 19:50:49-- http://0.0.0.0:39937/test.tar
Connecting to 0.0.0.0:39937... connected.
HTTP request sent, awaiting response... 200 OK
Length: 22 [text/html]
Saving to: ‘test.tar’

     0K 100% 4.74M=0s

2015-08-27 19:50:49 (4.74 MB/s) - ‘test.tar’ saved [22/22]

.E.................Warning: unable to find "test_relexepath" in the path
.............E
======================================================================
ERROR: snapcraft.tests.test_cmds (unittest.loader.ModuleImportFailure)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python3.4/unittest/case.py", line 57, in testPartExecutor
    yield
  File "/usr/lib/python3.4/unittest/case.py", line 574, in run
    testMethod()
  File "/usr/lib/python3.4/unittest/loader.py", line 32, in testFailure
    raise exception
ImportError: Failed to import test module: snapcraft.tests.test_cmds
Traceback (most recent call last):
  File "/usr/lib/python3.4/unittest/loader.py", line 312, in _find_tests
    module = self._get_module_from_name(name)
  File "/usr/lib/python3.4/unittest/loader.py", line 290, in _get_module_from_name
    __import__(name)
  File "/tmp/tarmac/branch.04EPnA/snapcraft/tests/test_cmds.py", line 24, in <module>
    from snapcraft import (
  File "/tmp/tarmac/branch.04EPnA/snapcraft/cmds.py", line 27, in <module>
    import snapcraft.yaml
  File "/tmp/tarmac/branch.04EPnA/snapcraft/yaml.py", line 21, in <module>
    import jsonschema
ImportError: No module named 'jsonschema'

======================================================================
ERROR: snapcraft.tests.test_yaml (unittest.loader.ModuleImportFailure)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python3.4/unittest/case.py", line 57, in testPartExecutor
    yield
  File "/usr/lib/python3.4/unittest/case.py", line 574, in run
    testMethod()
  File "/usr/lib/python3.4/unittest/loader.py", line 32, in testFailure
    raise exception
ImportError: Failed to import test module: snapcraft.tests.test_yaml
Traceback (most recent call last):
  File "/usr/lib/python3.4/unittest/loader.py", line 312, in _find_tests
    module = self._get_module_from_name(name)
  File "/usr/lib/python3.4/unittest/loader.py", line 290, in _get_module_from_name
    __import__(name)
  File "/tmp/tarmac/branch.04EPnA/snapcraft/tests/test_yaml.py", line 17, in <module>
    import jsonschema
ImportError: No module named 'jsonschema'

----------------------------------------------------------------------
Ran 44 tests in 1.155s

FAILED (errors=2)

[Sergio Schvezov (sergiusens)]

On Thu, Aug 27, 2015 at 4:47 PM, Leo Arias <email address hidden> wrote:

> Once this lands, the guys from erle and spreed have to be notified about
> the new format, right?

I've disabled daily builds for now

=== modified file 'debian/control'
--- debian/control	2015-08-07 19:49:21 +0000
+++ debian/control	2015-08-27 16:38:10 +0000
@@ -11,9 +11,10 @@
                pep8,
                plainbox,
                pyflakes,
-               python3 (>= 3.2),
+               python3 (>= 3.4),
                python3-apt,
                python3-fixtures,
+               python3-jsonschema,
                python3-setuptools,
                python3-testscenarios,
                python3-yaml,
@@ -33,6 +34,7 @@
          dpkg-dev,
          git,
          python3-apt,
+         python3-jsonschema,
          python3-yaml,
          mercurial,
          sudo,
 
=== modified file 'integration-tests/data/local-plugin/snapcraft.yaml'
--- integration-tests/data/local-plugin/snapcraft.yaml	2015-08-27 16:38:09 +0000
+++ integration-tests/data/local-plugin/snapcraft.yaml	2015-08-27 16:38:10 +0000
@@ -6,3 +6,4 @@
 
 parts:
     x-local-plugin:
+      source: .
 
=== added directory 'schema'
=== added file 'schema/snapcraft.yaml'
--- schema/snapcraft.yaml	1970-01-01 00:00:00 +0000
+++ schema/snapcraft.yaml	2015-08-27 16:38:10 +0000
@@ -0,0 +1,89 @@
+$schema: http://json-schema.org/draft-04/schema#
+
+title: snapcraft schema
+type: object
+properties:
+  name:
+    type: string
+    description: name of the snap package
+    pattern: "^[a-z0-9][a-z0-9+-]*$"
+  version:
+    # python's defaul yaml loading code loads 1.0 as an int
+    # type: string
+    description: package version
+    pattern: "^[a-zA-Z0-9.+~-]*$"
+  vendor:
+    type: string
+    format: email
+  summary:
+    type: string
+    description: one line summary for the package
+    maxLength: 78
+  description:
+    type: string
+    description: long description of the package
+  type:
+    type: string
+    description: the snap type, the implicit type is 'app'
+    enum:
+      - app
+      - framework
+  frameworks:
+    type: array
+    minItems: 1
+    uniqueItems: true
+    items:
+      - type: string
+  services:
+    type: array
+    items:
+      - type: object
+        properties:
+          name:
+            type: string
+          start:
+            type: string
+            description: command executed to start the service
+          stop:
+            type: string
+            description: command executed to stop the service
+        required:
+          - name
+          - start
+  binaries:
+    type: array
+    items:
+      - type: object
+        properties:
+          name:
+            type: string
+          exec:
+            type: string
+            description: command executed to start the service
+        required:
+          - name
+  parts:
+    type: object
+    minProperties: 1
+    patternProperties:
+      ^[a-z0-9][a-z0-9+-]*$:
+        type: object
+        properties:
+          plugin:
+            type: string
+            description: plugin name
+          source:
+            type: string
+            description: path to the sources
+        # TODO To be enabled once the Ubuntu plugin is removed
+        # required:
+          # - plugin
+          # - source
+    addtionalProperties: false
+required:
+  - name
+  - version
+  - vendor
+  - summary
+  - description
+  - parts
 
=== modified file 'setup.py'
--- setup.py	2015-07-06 16:48:22 +0000
+++ setup.py	2015-08-27 16:38:10 +0000
@@ -36,6 +36,9 @@
                 'snapcraft.plugins'],
       package_data={'snapcraft.plugins': ['manifest.txt']},
       scripts=['bin/snapcraft'],
-      data_files=[('share/snapcraft/plugins', ['plugins/' + x for x in os.listdir('plugins')])],
+      data_files=[
+          ('share/snapcraft/plugins', ['plugins/' + x for x in os.listdir('plugins')]),
+          ('share/snapcraft/schema', ['schema/' + x for x in os.listdir('schema')]),
+      ],
       cmdclass={'test': TestCommand},
-)
+      )
 
=== modified file 'snapcraft/common.py'
--- snapcraft/common.py	2015-08-05 15:39:18 +0000
+++ snapcraft/common.py	2015-08-27 16:38:10 +0000
@@ -25,6 +25,8 @@
 COMMAND_ORDER = ["pull", "build", "stage", "snap"]
 _DEFAULT_PLUGINDIR = '/usr/share/snapcraft/plugins'
 _plugindir = _DEFAULT_PLUGINDIR
+_DEFAULT_SCHEMADIR = '/usr/share/snapcraft/schema'
+_schemadir = _DEFAULT_SCHEMADIR
 _arch = None
 _arch_triplet = None
 
@@ -79,3 +81,12 @@
 
 def get_plugindir():
     return _plugindir
+
+
+def set_schemadir(schemadir):
+    global _schemadir
+    _schemadir = schemadir
+
+
+def get_schemadir():
+    return _schemadir
 
=== modified file 'snapcraft/dirs.py'
--- snapcraft/dirs.py	2015-07-23 17:19:25 +0000
+++ snapcraft/dirs.py	2015-08-27 16:38:10 +0000
@@ -27,3 +27,4 @@
     # only change the default if we are running from a checkout
     if os.path.exists(os.path.join(topdir, "setup.py")):
         common.set_plugindir(os.path.join(topdir, 'plugins'))
+        common.set_schemadir(os.path.join(topdir, 'schema'))
 
=== modified file 'snapcraft/tests/__init__.py'
--- snapcraft/tests/__init__.py	2015-08-04 15:06:59 +0000
+++ snapcraft/tests/__init__.py	2015-08-27 16:38:10 +0000
@@ -32,3 +32,4 @@
         # is a module variable. Make sure that it is returned to the original
         # value when a test ends.
         self.addCleanup(common.set_plugindir, common.get_plugindir())
+        self.addCleanup(common.set_schemadir, common.get_schemadir())
 
=== modified file 'snapcraft/tests/test_yaml.py'
--- snapcraft/tests/test_yaml.py	2015-08-05 18:40:06 +0000
+++ snapcraft/tests/test_yaml.py	2015-08-27 16:38:10 +0000
@@ -14,15 +14,18 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import jsonschema
 import logging
 import os
 import tempfile
 import unittest
+import unittest.mock
 
 import fixtures
 
+import snapcraft.common
+import snapcraft.yaml
 from snapcraft import dirs
-from snapcraft.yaml import Config
 from snapcraft.tests import TestCase
 
 
@@ -37,11 +40,19 @@
 
     @unittest.mock.patch('snapcraft.yaml.Config.load_plugin')
     def test_config_loads_plugins(self, mock_loadPlugin):
-        self.make_snapcraft_yaml("""parts:
+        dirs.setup_dirs()
+
+        self.make_snapcraft_yaml("""name: test
+version: "1"
+vendor: me <[e-mail]>
+summary: test
+description: test
+
+parts:
   ubuntu:
     packages: [fswebcam]
 """)
-        Config()
+        snapcraft.yaml.Config()
         mock_loadPlugin.assert_called_with("ubuntu", "ubuntu", {
             "packages": ["fswebcam"],
         })
@@ -52,7 +63,7 @@
 
         # no snapcraft.yaml
         with self.assertRaises(SystemExit) as raised:
-            Config()
+            snapcraft.yaml.Config()
 
         self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
         self.assertEqual(
@@ -66,7 +77,13 @@
         fake_logger = fixtures.FakeLogger(level=logging.ERROR)
         self.useFixture(fake_logger)
 
-        self.make_snapcraft_yaml("""parts:
+        self.make_snapcraft_yaml("""name: test
+version: "1"
+vendor: me <[e-mail]>
+summary: test
+description: test
+
+parts:
   p1:
     plugin: ubuntu
     after: [p2]
@@ -75,7 +92,235 @@
     after: [p1]
 """)
         with self.assertRaises(SystemExit) as raised:
-            Config()
+            snapcraft.yaml.Config()
 
         self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
         self.assertEqual('Circular dependency chain!\n', fake_logger.output)
+
+    @unittest.mock.patch('snapcraft.yaml.Config.load_plugin')
+    def test_invalid_yaml_missing_name(self, mock_loadPlugin):
+        dirs.setup_dirs()
+
+        fake_logger = fixtures.FakeLogger(level=logging.ERROR)
+        self.useFixture(fake_logger)
+
+        self.make_snapcraft_yaml("""
+version: "1"
+vendor: me <[e-mail]>
+summary: test
+description: nothing
+
+parts:
+  ubuntu:
+    packages: [fswebcam]
+""")
+        with self.assertRaises(SystemExit) as raised:
+            snapcraft.yaml.Config()
+
+        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
+        self.assertEqual(
+            'Issues while validating snapcraft.yaml: \'name\' is a required property\n',
+            fake_logger.output)
+
+    @unittest.mock.patch('snapcraft.yaml.Config.load_plugin')
+    def test_invalid_yaml_invalid_name_as_number(self, mock_loadPlugin):
+        dirs.setup_dirs()
+
+        fake_logger = fixtures.FakeLogger(level=logging.ERROR)
+        self.useFixture(fake_logger)
+
+        self.make_snapcraft_yaml("""name: 1
+version: "1"
+vendor: me <[e-mail]>
+summary: test
+description: nothing
+
+parts:
+  ubuntu:
+    packages: [fswebcam]
+""")
+        with self.assertRaises(SystemExit) as raised:
+            snapcraft.yaml.Config()
+
+        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
+        self.assertEqual(
+            'Issues while validating snapcraft.yaml: 1 is not of type \'string\'\n',
+            fake_logger.output)
+
+    @unittest.mock.patch('snapcraft.yaml.Config.load_plugin')
+    def test_invalid_yaml_invalid_name_chars(self, mock_loadPlugin):
+        dirs.setup_dirs()
+
+        fake_logger = fixtures.FakeLogger(level=logging.ERROR)
+        self.useFixture(fake_logger)
+
+        self.make_snapcraft_yaml("""name: myapp@me_1.0
+version: "1"
+vendor: me <[e-mail]>
+summary: test
+description: nothing
+
+parts:
+  ubuntu:
+    packages: [fswebcam]
+""")
+        with self.assertRaises(SystemExit) as raised:
+            snapcraft.yaml.Config()
+
+        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
+        self.assertEqual(
+            'Issues while validating snapcraft.yaml: \'myapp@me_1.0\' does not match \'^[a-z0-9][a-z0-9+-]*$\'\n',
+            fake_logger.output)
+
+    @unittest.mock.patch('snapcraft.yaml.Config.load_plugin')
+    def test_invalid_yaml_missing_description(self, mock_loadPlugin):
+        dirs.setup_dirs()
+
+        fake_logger = fixtures.FakeLogger(level=logging.ERROR)
+        self.useFixture(fake_logger)
+
+        self.make_snapcraft_yaml("""name: test
+version: "1"
+vendor: me <[e-mail]>
+summary: test
+
+parts:
+  ubuntu:
+    packages: [fswebcam]
+""")
+        with self.assertRaises(SystemExit) as raised:
+            snapcraft.yaml.Config()
+
+        self.assertEqual(raised.exception.code, 1, 'Wrong exit code returned.')
+        self.assertEqual(
+            'Issues while validating snapcraft.yaml: \'description\' is a required property\n',
+            fake_logger.output)
+
+
+class TestValidation(TestCase):
+
+    def setUp(self):
+        super().setUp()
+        dirs.setup_dirs()
+
+        self.data = {
+            'name': 'my-package-1',
+            'version': '1.0-snapcraft1~ppa1',
+            'vendor': 'Me <[e-mail]>',
+            'summary': 'my summary less that 79 chars',
+            'description': 'description which can be pretty long',
+            'parts': {
+                'part1': {
+                    'type': 'project',
+                },
+            },
+        }
+
+    def test_required_properties(self):
+        for key in self.data:
+            data = self.data.copy()
+            with self.subTest(key=key):
+                del data[key]
+
+                with self.assertRaises(jsonschema.ValidationError) as raised:
+                    snapcraft.yaml._validate_snapcraft_yaml(data)
+
+                expected_message = '\'{}\' is a required property'.format(key)
+                self.assertEqual(raised.exception.message, expected_message, msg=data)
+
+    def test_invalid_names(self):
+        invalid_names = [
+            'package@awesome',
+            'something.another',
+            '_hideme',
+        ]
+
+        for name in invalid_names:
+            data = self.data.copy()
+            with self.subTest(key=name):
+                data['name'] = name
+
+                with self.assertRaises(jsonschema.ValidationError) as raised:
+                    snapcraft.yaml._validate_snapcraft_yaml(data)
+
+                expected_message = '\'{}\' does not match \'^[a-z0-9][a-z0-9+-]*$\''.format(name)
+                self.assertEqual(raised.exception.message, expected_message, msg=data)
+
+    def test_summary_too_long(self):
+        self.data['summary'] = 'a' * 80
+        with self.assertRaises(jsonschema.ValidationError) as raised:
+            snapcraft.yaml._validate_snapcraft_yaml(self.data)
+
+        expected_message = '\'{}\' is too long'.format(self.data['summary'])
+        self.assertEqual(raised.exception.message, expected_message, msg=self.data)
+
+    def test_valid_types(self):
+        self.data['type'] = 'app'
+        snapcraft.yaml._validate_snapcraft_yaml(self.data)
+
+        self.data['type'] = 'framework'
+        snapcraft.yaml._validate_snapcraft_yaml(self.data)
+
+    def test_invalid_types(self):
+        invalid_types = [
+            'apps',
+            'kernel',
+            'platform',
+            'oem',
+            'os',
+        ]
+
+        for t in invalid_types:
+            data = self.data.copy()
+            with self.subTest(key=t):
+                data['type'] = t
+
+                with self.assertRaises(jsonschema.ValidationError) as raised:
+                    snapcraft.yaml._validate_snapcraft_yaml(data)
+
+                expected_message = '\'{}\' is not one of [\'app\', \'framework\']'.format(t)
+                self.assertEqual(raised.exception.message, expected_message, msg=data)
+
+    def test_valid_services(self):
+        self.data['services'] = [
+            {
+                'name': 'service1',
+                'start': 'binary1 start',
+            },
+            {
+                'name': 'service2',
+                'start': 'binary2',
+                'stop': 'binary2 --stop',
+            },
+            {
+                'name': 'service3',
+            }
+        ]
+
+        snapcraft.yaml._validate_snapcraft_yaml(self.data)
+
+    def test_services_required_properties(self):
+        self.data['services'] = [
+            {
+                'start': 'binary1 start',
+            }
+        ]
+
+        with self.assertRaises(jsonschema.ValidationError) as raised:
+            snapcraft.yaml._validate_snapcraft_yaml(self.data)
+
+        expected_message = '\'name\' is a required property'
+        self.assertEqual(raised.exception.message, expected_message, msg=self.data)
+
+    def test_schema_file_not_found(self):
+        mock_the_open = unittest.mock.mock_open()
+        mock_the_open.side_effect = FileNotFoundError()
+
+        with unittest.mock.patch('snapcraft.yaml.open', mock_the_open, create=True):
+            with self.assertRaises(snapcraft.yaml.SchemaNotFoundError) as raised:
+                snapcraft.yaml._validate_snapcraft_yaml(self.data)
+
+        expected_path = os.path.join(snapcraft.common.get_schemadir(), 'snapcraft.yaml')
+        mock_the_open.assert_called_once_with(expected_path)
+        expected_message = 'Schema is missing, could not validate snapcraft.yaml, check installation'
+        self.assertEqual(raised.exception.message, expected_message)
 
=== modified file 'snapcraft/yaml.py'
--- snapcraft/yaml.py	2015-08-26 08:52:09 +0000
+++ snapcraft/yaml.py	2015-08-27 16:38:10 +0000
@@ -18,6 +18,8 @@
 import sys
 
 import yaml
+import jsonschema
+import os
 
 import snapcraft.plugin
 from snapcraft import common
@@ -26,6 +28,24 @@
 logger = logging.getLogger(__name__)
 
 
+class SchemaNotFoundError(Exception):
+
+    def __init__(self, message):
+        self.message = message
+
+
+def _validate_snapcraft_yaml(snapcraft_yaml):
+    schema_file = os.path.abspath(os.path.join(common.get_schemadir(), 'snapcraft.yaml'))
+
+    try:
+        with open(schema_file) as fp:
+            schema = yaml.load(fp)
+    except FileNotFoundError:
+        raise SchemaNotFoundError('Schema is missing, could not validate snapcraft.yaml, check installation')
+
+    jsonschema.validate(snapcraft_yaml, schema)
+
+
 class Config:
 
     def __init__(self):
@@ -39,6 +59,18 @@
         except FileNotFoundError:
             logger.error("Could not find snapcraft.yaml.  Are you sure you're in the right directory?\nTo start a new project, use 'snapcraft init'")
             sys.exit(1)
+
+        # Make sure the loaded snapcraft yaml follows the schema
+        try:
+            _validate_snapcraft_yaml(self.data)
+        except SchemaNotFoundError as e:
+            logger.error(e.message)
+            sys.exit(1)
+        except jsonschema.ValidationError as e:
+            msg = "Issues while validating snapcraft.yaml: {}".format(e.message)
+            logger.error(msg)
+            sys.exit(1)
+
         self.build_tools = self.data.get('build-tools', [])
 
         for part_name in self.data.get("parts", []):