Merge ~jslarraz/review-tools:container-add-get-yaml into review-tools:master

[Jorge Sancho Larraz]

many: add get_yaml to BaseContainer and use it to read snap.yaml and manifest.yaml

[Jorge Sancho Larraz (jslarraz)]

Add `get_yaml` function to base_container. This function will use ruamel.yaml to load a yaml file in the container and return the corresponding dictionary.

Using ruamel.yaml with `.allow_duplicate_keys = False` removes the need to perform this check separately.

A clean up has also been performed in sr_common using this new `get_yaml` function.

[Alex Murray (alexmurray)]

LGTM although I wonder a bit about hard-coding the duplicate keys logic and also perhaps since this method is wrapping get_file() then perhaps it should be called get_file_as_yaml() or something to indicate this...? although naming is always hard.

I am happy for you to merge this as is but would be interested to know your thoughts either way.

review: Approve

[Jorge Sancho Larraz (jslarraz)]

Regarding the naming, I don't think that the function name should indicate that it wraps `get_file` as it is an implementation detail that should be transparent for anyone using this function. Otherwise, changing the implementation would require to update the function name, what would break backwards compatibility. It should probable be better included in the documentation. That said, I'll be happy to update the name to `get_file_as_yaml` as it makes more evident that we are actually reading a file from the container and interpreting it as yaml.

Regarding duplicated keys, I just hard-coded them because yaml specification does not allow them and all cases where I started to use the `get_yaml` function, previously checked for duplicated keys with the function `_verify_no_duplicated_yaml_keys`, but I agree that it should be better handled via function arguments.

Working on that!

[Jorge Sancho Larraz (jslarraz)]

Comments addressed on https://git.launchpad.net/~jslarraz/review-tools/commit/?id=c89b50bcae4d7e7362550b5180f279f16cd93d7f

[Alex Murray (alexmurray)]

Thanks @jslarraz - still LGTM - fwiw I like the parameterised approach - very nice!

review: Approve

diff --git a/reviewtools/containers/base_container.py b/reviewtools/containers/base_container.py
index 0d51021..3c4555f 100644
--- a/reviewtools/containers/base_container.py
+++ b/reviewtools/containers/base_container.py
@@ -1,6 +1,7 @@
 import os
 import shutil
 import magic
+import ruamel.yaml
 from typing import Union
 from tempfile import mkdtemp
 
@@ -244,7 +245,7 @@ class BaseContainer:
         """
         raise NotImplementedError("Must be implemented by subclasses")
 
-    def get_file(self, fn: str) -> Union[str, None]:
+    def get_file(self, fn: str) -> bytes:
         """
         Reads and return the raw content of the specified file.
 
@@ -265,6 +266,47 @@ class BaseContainer:
         with open(os.path.join(self.unpack_dir, fn), "rb") as fd:
             return fd.read()
 
+    def get_file_as_yaml(
+        self, fn: str, yaml_version: str = "1.2", allow_duplicate_keys: bool = False
+    ) -> Union[dict, None]:
+        """
+        Reads a file and tries to parse it as a yaml file. It uses get_file() to
+        read the specified file from the container.
+
+        Args:
+            fn (str): file path relative to the container root (i.e. unpack_dir)
+            yaml_version (str): yaml specification version used to parse the file.
+                Defaults to ``1.2``
+            allow_duplicate_keys (bool): if ``True``, allow duplicate keys in the
+                file. Defaults to ``False``
+
+        Returns:
+            dict: file content parsed as yaml
+
+        Raises:
+            ContainerException: Calling this method will raise an exception if
+                the requested file is not found.
+        """
+        # Check parameters
+        if yaml_version not in ["1.1", "1.2"]:
+            raise ContainerException("yaml_version must be 1.1 or 1.2")
+
+        # Load file
+        file = self.get_file(fn)
+
+        # Prevent duplicated keys
+        yaml = ruamel.yaml.YAML(typ="safe")
+        yaml.allow_duplicate_keys = allow_duplicate_keys
+        # Octal integers, used by layout::mode are handle differently in yaml
+        # versions 1.1 (0755) and 1.2 (0o755).
+        yaml.version = yaml_version
+        try:
+            yaml_object = yaml.load(file.decode())
+        except ruamel.yaml.constructor.DuplicateKeyError as e:
+            raise ContainerException(e.problem)
+
+        return yaml_object
+
     def get_files_list(self, abs: bool = False) -> list:
         """
         Returns a list of files present in the container.
diff --git a/reviewtools/sr_common.py b/reviewtools/sr_common.py
index 9b961e2..5b2f2ec 100644
--- a/reviewtools/sr_common.py
+++ b/reviewtools/sr_common.py
@@ -17,14 +17,11 @@
 from __future__ import print_function
 import os
 import re
-import yaml
-
 
 from reviewtools.common import (
     ReviewBase,
     ReviewException,
     error,
-    open_file_read,
     read_snapd_base_declaration,
     unsquashfs_lln,
     unsquashfs_lln_parse,
@@ -408,32 +405,17 @@ class SnapReview(ReviewBase):
         self.pkg_bin_files = self.snap.get_compiled_binaries_list(abs=True)
         self.tmp_dir = self.snap.tmp_dir
 
-        snap_yaml = self._extract_snap_yaml()
-        try:
-            self.snap_yaml = yaml.safe_load(snap_yaml)
-        except Exception:  # pragma: nocover
-            error("Could not load snap.yaml. Is it properly formatted?")
-        snap_yaml.close()
-
-        # read yaml again for duplicated keys check (py-yaml does not
-        # do that)
-        snap_yaml = self._extract_snap_yaml()
         try:
-            self._verify_no_duplicated_yaml_keys(snap_yaml.read())
-        except Exception as e:
-            error("Found duplicated yaml keys in snap.yaml: %s" % e)
-        snap_yaml.close()
-
-        self.snap_manifest_yaml = {}
-        manifest_yaml = self._extract_snap_manifest_yaml()
-        if manifest_yaml is not None:
-            try:
-                self.snap_manifest_yaml = yaml.safe_load(manifest_yaml)
-                manifest_yaml.close()
-                if self.snap_manifest_yaml is None:
-                    self.snap_manifest_yaml = {}
-            except Exception:  # pragma: nocover
-                error("Could not load snap/manifest.yaml. Is it properly " "formatted?")
+            self.snap_yaml = self.snap.get_file_as_yaml(
+                "meta/snap.yaml", yaml_version="1.1"
+            )
+            self.snap_manifest_yaml = {}
+            if "snap/manifest.yaml" in self.snap.files:
+                self.snap_manifest_yaml = self.snap.get_file_as_yaml(
+                    "snap/manifest.yaml", yaml_version="1.1"
+                )
+        except ContainerException as e:
+            error(str(e))
 
         (
             self.base_declaration_series,
@@ -522,24 +504,6 @@ class SnapReview(ReviewBase):
         # cache unsquashfs -lln so we can use it all over
         self.unsquashfs_lln_hdr, self.unsquashfs_lln_entries = self._unsquashfs_lln(fn)
 
-    # Since coverage is looked at via the testsuite and the testsuite mocks
-    # this out, don't cover this
-    def _extract_snap_yaml(self):  # pragma: nocover
-        """Extract and read the snappy 16.04 snap.yaml"""
-        y = os.path.join(self.unpack_dir, "meta/snap.yaml")
-        if not os.path.isfile(y):
-            error("Could not find snap.yaml.")
-        return open_file_read(y)
-
-    # Since coverage is looked at via the testsuite and the testsuite mocks
-    # this out, don't cover this
-    def _extract_snap_manifest_yaml(self):  # pragma: nocover
-        """Extract and read snap/manifest.yaml if it exists"""
-        y = os.path.join(self.unpack_dir, "snap/manifest.yaml")
-        if not os.path.isfile(y):
-            return None
-        return open_file_read(y)
-
     def _unsquashfs_lln(self, snap_pkg):
         """Run unsquashfs -lln on a snap package"""
         (rc, out) = unsquashfs_lln(snap_pkg)
@@ -632,14 +596,3 @@ class SnapReview(ReviewBase):
         if os.path.getsize(fn) <= size:
             return True
         return False
-
-    def _verify_no_duplicated_yaml_keys(self, raw_snap_yaml):
-        """Verify there are no duplicated yaml map keys"""
-        import ruamel.yaml
-
-        yaml = ruamel.yaml.YAML(typ="safe")
-        yaml.allow_duplicate_keys = False
-        try:
-            yaml.load(raw_snap_yaml)
-        except ruamel.yaml.constructor.DuplicateKeyError as e:
-            raise SnapReviewException(e.problem)
diff --git a/reviewtools/sr_tests.py b/reviewtools/sr_tests.py
index bfbf375..eaea513 100644
--- a/reviewtools/sr_tests.py
+++ b/reviewtools/sr_tests.py
@@ -15,9 +15,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import copy
-import io
 import os
-import yaml
 
 from unittest.mock import patch
 from unittest import TestCase
@@ -46,19 +44,17 @@ class _BaseContainer:
     def __init__(self, fn):
         self.filename = fn
         self._unpack_dir = ".".join(self.filename.split(".")[:-1])
-        self._files = []
+        self._files = ["meta/snap.yaml", "snap/manifest.yaml"]
         self._bin_files = []
         self.tmp_dir = "/faketmpdir"
 
-
-def _extract_snap_yaml(self):
-    """Pretend we read the snap.yaml file"""
-    return io.StringIO(TEST_SNAP_YAML)
-
-
-def _extract_snap_manifest_yaml(self):
-    """Pretend we read the snap/manifest.yaml file"""
-    return io.StringIO(TEST_SNAP_MANIFEST_YAML)
+    def get_file_as_yaml(self, fn, yaml_version="1.2", allow_duplicate_keys=False):
+        if fn == "meta/snap.yaml":
+            return TEST_SNAP_YAML
+        elif fn == "snap/manifest.yaml":
+            return TEST_SNAP_MANIFEST_YAML
+        else:
+            return {}
 
 
 def _pkgfmt_type(self):
@@ -90,12 +86,9 @@ def create_patches():
         patch("reviewtools.sr_common.SnapContainer.__init__", _BaseContainer.__init__)
     )
     patches.append(
-        patch("reviewtools.sr_common.SnapReview._extract_snap_yaml", _extract_snap_yaml)
-    )
-    patches.append(
         patch(
-            "reviewtools.sr_common.SnapReview._extract_snap_manifest_yaml",
-            _extract_snap_manifest_yaml,
+            "reviewtools.sr_common.SnapContainer.get_file_as_yaml",
+            _BaseContainer.get_file_as_yaml,
         )
     )
     # sr_common
@@ -141,15 +134,11 @@ class TestSnapReview(TestCase):
 
     def _update_test_snap_yaml(self):
         global TEST_SNAP_YAML
-        TEST_SNAP_YAML = yaml.dump(
-            self.test_snap_yaml, default_flow_style=False, indent=4
-        )
+        TEST_SNAP_YAML = self.test_snap_yaml
 
     def _update_test_snap_manifest_yaml(self):
         global TEST_SNAP_MANIFEST_YAML
-        TEST_SNAP_MANIFEST_YAML = yaml.dump(
-            self.test_snap_manifest_yaml, default_flow_style=False, indent=4
-        )
+        TEST_SNAP_MANIFEST_YAML = self.test_snap_manifest_yaml
 
     def _update_test_name(self):
         self.test_name = "%s.origin_%s_%s.snap" % (
diff --git a/reviewtools/tests/containers/test_base_container.py b/reviewtools/tests/containers/test_base_container.py
index 1b4e022..277be8f 100644
--- a/reviewtools/tests/containers/test_base_container.py
+++ b/reviewtools/tests/containers/test_base_container.py
@@ -35,7 +35,15 @@ class TestBaseContainer(unittest.TestCase):
             os.makedirs(os.path.dirname(unpacked_file), exist_ok=True)
             if file not in self.expected_binaries:
                 with open(unpacked_file, "w+b") as fd:
-                    fd.write(b"Test")
+                    if file == "meta/snap.yaml":
+                        fd.write(
+                            b"""
+map:
+ key: value
+                        """
+                        )
+                    else:
+                        fd.write(b"Test")
             else:
                 shutil.copyfile(os.path.join("/", file), unpacked_file)
         return unpack_dir
@@ -126,10 +134,10 @@ class TestBaseContainer(unittest.TestCase):
         try:
             container = BaseContainer(self.fn)
             container._unpack_dir = self.unpack_dir
-            snap_yaml = container.get_file("meta/snap.yaml")
+            snap_yaml = container.get_file("meta/icon.png")
             self.assertEqual(snap_yaml.decode(), "Test")
         except ContainerException:
-            self.fail("An unexpected error occurred while getting meta/snap.yaml")
+            self.fail("An unexpected error occurred while getting meta/icon.png")
 
     def test_get_file__missing_file(self):
         with self.assertRaises(ContainerException) as context:
@@ -138,6 +146,88 @@ class TestBaseContainer(unittest.TestCase):
             container.get_file("non/existent/file")
             self.assertTrue("Could not find" in context.exception.value)
 
+    # Test get yaml
+    def test_get_file_as_yaml__happy(self):
+        try:
+            container = BaseContainer(self.fn)
+            container._unpack_dir = self.unpack_dir
+            snap_yaml = container.get_file_as_yaml("meta/snap.yaml")
+            self.assertEqual(snap_yaml, {"map": {"key": "value"}})
+        except ContainerException:
+            self.fail("An unexpected error occurred while getting meta/snap.yaml")
+
+    def test_get_file_as_yaml__dup_keys(self):
+        # Write the file to test
+        with open(os.path.join(self.unpack_dir, "test.yaml"), "w+b") as fd:
+            fd.write(
+                b"""
+map:
+  key: value1
+  key: value2
+    """
+            )
+
+        with self.assertRaises(ContainerException) as context:
+            container = BaseContainer(self.fn)
+            container._unpack_dir = self.unpack_dir
+            container.get_file_as_yaml("test.yaml")
+            self.assertTrue('found duplicate key "key"' in context.exception.value)
+
+        # Clean the tested file
+        os.unlink(os.path.join(self.unpack_dir, "test.yaml"))
+
+    def test_get_file_as_yaml__allow_dup_keys(self):
+        # Write the file to test
+        with open(os.path.join(self.unpack_dir, "test.yaml"), "w+b") as fd:
+            fd.write(
+                b"""
+map:
+  key: value1
+  key: value2
+    """
+            )
+
+        container = BaseContainer(self.fn)
+        container._unpack_dir = self.unpack_dir
+        container.get_file_as_yaml("test.yaml", allow_duplicate_keys=True)
+
+        # Clean the tested file
+        os.unlink(os.path.join(self.unpack_dir, "test.yaml"))
+
+    def test_get_file_as_yaml__version_1_1(self):
+        # Write the file to test
+        with open(os.path.join(self.unpack_dir, "test.yaml"), "w+b") as fd:
+            fd.write(
+                b"""
+mode: 0777
+    """
+            )
+
+        container = BaseContainer(self.fn)
+        container._unpack_dir = self.unpack_dir
+        test_yaml = container.get_file_as_yaml("test.yaml", yaml_version="1.1")
+        self.assertEqual(test_yaml["mode"], 511)
+
+        # Clean the tested file
+        os.unlink(os.path.join(self.unpack_dir, "test.yaml"))
+
+    def test_get_file_as_yaml__version_1_2(self):
+        # Write the file to test
+        with open(os.path.join(self.unpack_dir, "test.yaml"), "w+b") as fd:
+            fd.write(
+                b"""
+mode: 0777
+    """
+            )
+
+        container = BaseContainer(self.fn)
+        container._unpack_dir = self.unpack_dir
+        test_yaml = container.get_file_as_yaml("test.yaml")
+        self.assertEqual(test_yaml["mode"], 777)
+
+        # Clean the tested file
+        os.unlink(os.path.join(self.unpack_dir, "test.yaml"))
+
     # Test get files list
     def test_get_files_list__happy(self):
         try:
diff --git a/reviewtools/tests/test_bbb_example_sr_skeleton.py b/reviewtools/tests/test_bbb_example_sr_skeleton.py
index 5a304cd..0abf94e 100644
--- a/reviewtools/tests/test_bbb_example_sr_skeleton.py
+++ b/reviewtools/tests/test_bbb_example_sr_skeleton.py
@@ -63,7 +63,6 @@ class TestSnapReviewSkeleton(sr_tests.TestSnapReview):
         self._update_test_name()
 
         import pprint
-        import yaml
 
         print(
             """
@@ -75,4 +74,4 @@ class TestSnapReviewSkeleton(sr_tests.TestSnapReview):
 """
             % (self.test_name)
         )
-        pprint.pprint(yaml.load(sr_tests.TEST_SNAP_YAML, Loader=yaml.SafeLoader))
+        pprint.pprint(sr_tests.TEST_SNAP_YAML)
diff --git a/reviewtools/tests/test_sr_common.py b/reviewtools/tests/test_sr_common.py
 deleted file mode 100644
index 541cea9..0000000
--- a/reviewtools/tests/test_sr_common.py
+++ /dev/null
@@ -1,38 +0,0 @@
-"""test_sr_common.py: tests for the sr_common module"""
-#
-# Copyright (C) 2018 Canonical Ltd.
-#
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation; version 3 of the License.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-from reviewtools.sr_common import SnapReview
-import reviewtools.sr_tests as sr_tests
-
-
-class TestSnapReviewCommon(sr_tests.TestSnapReview):
-    def setUp(self):
-        super().setUp()
-        self.review = SnapReview("app.snap", "sr_common_review_type")
-
-    def test_no_duplicated_yaml_keys(self):
-        """Check _no_duplicated_yaml_keys"""
-        b1 = """
-map:
- key:
- key:
-        """
-        for ok in [
-            b1,
-        ]:
-            with self.assertRaises(Exception) as e:
-                self.review._verify_no_duplicated_yaml_keys(ok)
-            self.assertRegex(str(e.exception), r'found duplicate key "key".*')