Merge pull request #845 from getpopper/parser-fixes

Parser fixes

.gitignore

@@ -16,4 +16,4 @@ popper_status
 popper_logs
 popper_status
 .pipeline_cache.yml
-cli/popper/_version.py
+cli/popper/_version.py

cli/popper/__init__.py

@@ -16,6 +16,6 @@
     _init_script_dir = pathlib.Path(__file__).parent.absolute()
     _version_path_ = os.path.join(_init_script_dir, "_version.py")
     with open(_version_path_, "w") as v:
-        v.write(_ver)
+        v.write(_ver + "\n")
 else:
     from popper._version import __popper_version__ as __version__

cli/popper/commands/cmd_ci.py

@@ -100,9 +100,9 @@
     type=click.Choice(["travis", "circle", "jenkins", "gitlab", "brigade"]),
     required=True,
 )
-@click.option("-f", "--wfile", help="Specify workflow to run in CI.", required=True)
+@click.option("-f", "--file", help="Specify workflow to run in CI.", required=True)
 @pass_context
-def cli(ctx, service, wfile):
+def cli(ctx, service, file):
     """Generates configuration files for distinct CI services. This command
     needs to be executed on the root of your Git repository folder.
     """
@@ -116,6 +116,6 @@ def cli(ctx, service, wfile):
         ci_file = os.path.join(os.getcwd(), ci_file)
         os.makedirs(os.path.dirname(ci_file), exist_ok=True)
         with open(ci_file, "w") as f:
-            f.write(ci_file_content.format(wfile))
+            f.write(ci_file_content.format(file))
 
     log.info(f"Wrote {service} configuration successfully.")

cli/popper/commands/cmd_dot.py

@@ -6,7 +6,7 @@
 
 @click.option(
     "-f",
-    "--wfile",
+    "--file",
     help="File containing the definition of the workflow.",
     required=True,
 )
@@ -22,9 +22,9 @@
 )
 @click.command("dot", short_help="Generate a graph in the .dot format.")
 @pass_context
-def cli(ctx, wfile, skip, colors):
+def cli(ctx, file, skip, colors):
     """Creates a graph in the .dot format representing the workflow."""
-    wf = WorkflowParser.parse(file=wfile)
+    wf = WorkflowParser.parse(file=file)
 
     node_attrs = 'shape=box, style="filled{}", fillcolor=transparent{}'
     wf_attr = node_attrs.format(",rounded", ",color=red" if colors else "")

cli/popper/commands/cmd_run.py

@@ -13,9 +13,10 @@
 @click.argument("step", required=False)
 @click.option(
     "-f",
-    "--wfile",
+    "--file",
     help="File containing the definition of the workflow.",
-    required=True,
+    required=False,
+    default=".popper.yml",
 )
 @click.option(
     "-d",
@@ -109,7 +110,7 @@
 def cli(
     ctx,
     step,
-    wfile,
+    file,
     debug,
     dry_run,
     log_file,
@@ -159,7 +160,7 @@ def cli(
 
     # invoke wf factory; handles formats, validations, filtering
     wf = WorkflowParser.parse(
-        wfile,
+        file,
         step=step,
         skipped_steps=skip,
         substitutions=substitution,

cli/popper/commands/cmd_scaffold.py

@@ -8,13 +8,13 @@
 @click.command("scaffold", short_help="Generate a minimal workflow.")
 @click.option(
     "-f",
-    "--wfile",
+    "--file",
     help="Name of file where to write the generated workflow.",
     required=False,
     default="wf.yml",
 )
 @pass_context
-def cli(ctx, wfile):
+def cli(ctx, file):
     """Generates a minimal workflow that can be used as starting point."""
     main_workflow_content = """steps:
 
@@ -25,10 +25,10 @@ def cli(ctx, wfile):
   args: ["ls"]
 """
 
-    if os.path.exists(wfile):
-        log.fail(f"File {wfile} already exists")
+    if os.path.exists(file):
+        log.fail(f"File {file} already exists")
 
-    with open(wfile, "w") as f:
+    with open(file, "w") as f:
         f.write(main_workflow_content)
 
     log.info("Successfully generated a workflow scaffold.")

cli/popper/parser.py

@@ -84,16 +84,18 @@ def parse(
                 if not _wf_data:
                     log.fail(f"File {file} is empty")
         else:
-            _wf_data = wf_data
+            _wf_data = dict(wf_data)
 
-        # hack to silence warnings about error to fail change
+        # disable logging in order to silence warnings about "error() to fail()" change
         logging.disable(logging.CRITICAL)
 
         v = YMLValidator(source_data=_wf_data, schema_data=WorkflowParser._wf_schema)
 
         try:
             v.validate()
         except SchemaError as e:
+            # reenable logging
+            logging.disable(logging.NOTSET)
             log.fail(f"{e.msg}")
 
         logging.disable(logging.NOTSET)
@@ -117,25 +119,30 @@ def parse(
     def __apply_substitution(wf_element, k, v, used_registry):
         if isinstance(wf_element, str):
             if k in wf_element:
-                wf_element.replace(k, v)
+                log.debug(f"Applying substitution to string {k}")
+                wf_element = wf_element.replace(k, v)
                 used_registry[k] = 1
 
         elif isinstance(wf_element, list):
             # we assume list of strings
             for i, e in enumerate(wf_element):
                 if k in e:
-                    wf_element[i].replace(k, v)
+                    log.debug(f"Applying substitution to item {i}: {e}")
+                    wf_element[i] = wf_element[i].replace(k, v)
                     used_registry[k] = 1
 
         elif isinstance(wf_element, dict):
-            # we assume list of strings
+            # we assume map of strings
             for ek in wf_element:
                 if k in ek:
-                    log.fail("Substitutions only allowed on keys of dictionaries")
+                    log.fail("Substitutions not allowed on dictionary keys")
                 if k in wf_element[ek]:
-                    wf_element[ek].replace(k, v)
+                    log.debug(f"Applying substitution to value associated to key {k}")
+                    wf_element[ek] = wf_element[ek].replace(k, v)
                     used_registry[k] = 1
 
+        return wf_element
+
     @staticmethod
     def __add_missing_ids(wf_data):
         for i, step in enumerate(wf_data["steps"]):
@@ -171,21 +178,28 @@ def __apply_substitutions(wf_data, substitutions=None, allow_loose=False):
             if len(item) < 2:
                 raise Exception("Excepting '=' as seperator")
 
-            k, v = ("$" + item[0], item[1])
+            k, v = (item[0], item[1])
+
+            if not re.match(r"_[A-Z0-9]+", k):
+                log.fail(f"Expecting substitution key in _[A-Z0-9] format; got '{k}'.")
 
-            if not re.match(r"\$_[A-Z0-9]+", k):
-                log.fail(f"Expecting substitution key as $_[A-Z0-9] but got '{k}'.")
+            # add dollar sign
+            k = "$" + k
 
             # replace in steps
             for step in wf_data["steps"]:
-                for _, step_attr in step.items():
-                    WorkflowParser.__apply_substitution(step_attr, k, v, used)
-
-            for _, options_attr in wf_data.get("options", {}).items():
-                WorkflowParser.__apply_substitution(options_attr, k, v, used)
+                for attr in step:
+                    step[attr] = WorkflowParser.__apply_substitution(
+                        step[attr], k, v, used
+                    )
+            opts = wf_data.get("options", {})
+            for attr in opts:
+                opts[attr] = WorkflowParser.__apply_substitution(opts[attr], k, v, used)
 
         if not allow_loose and len(substitutions) != len(used):
-            log.fail("Not all given substitutions are used in " "the workflow file")
+            log.fail("Not all given substitutions are used in the workflow file")
+
+        log.debug(f"Workflow after applying substitutions: {wf_data}")
 
     @staticmethod
     def __skip_steps(wf_data, skip_list=[]):
@@ -201,7 +215,7 @@ def __skip_steps(wf_data, skip_list=[]):
         wf_data["steps"] = filtered_list
 
         if len(used) != len(skip_list):
-            log.fail(f"Not all skipped steps exist in the workflow.")
+            log.fail("Not all skipped steps exist in the workflow.")
 
     @staticmethod
     def __filter_step(wf_data, filtered_step=None):
@@ -210,4 +224,6 @@ def __filter_step(wf_data, filtered_step=None):
             return
         for step in wf_data["steps"]:
             if step["id"] == filtered_step:
-                return step
+                wf_data["steps"] = [step]
+                return
+        log.fail(f"Step {filtered_step} not in workflow")

cli/test/test_parser.py

@@ -1,11 +1,8 @@
 import unittest
-import os
 
 from popper.parser import WorkflowParser
 from popper.cli import log
 
-import logging
-
 
 class TestWorkflow(unittest.TestCase):
     def setUp(self):
@@ -61,9 +58,96 @@ def test_new_workflow(self):
         self.assertEqual({"FOO": "bar"}, wf.options.env)
         self.assertEqual(("Z",), wf.options.secrets)
 
+    def test_filter_all_but_given_step(self):
+        wf_data = {
+            "steps": [
+                {"uses": "foo", "id": "one"},
+                {"uses": "bar", "id": "two"},
+                {"uses": "baz", "id": "three"},
+            ]
+        }
+        wf = WorkflowParser.parse(wf_data=wf_data, step="two")
+        self.assertEqual(1, len(wf.steps))
+        self.assertEqual("two", wf.steps[0].id)
+        self.assertEqual("bar", wf.steps[0].uses)
+
+        # non-existing name
+        self.assertRaises(
+            SystemExit, WorkflowParser.parse, **{"wf_data": wf_data, "step": "four"}
+        )
+
+    def test_skip_steps(self):
+        wf_data = {
+            "steps": [
+                {"uses": "foo", "id": "one"},
+                {"uses": "bar", "id": "two"},
+                {"uses": "baz", "id": "three"},
+            ]
+        }
+        # skip one step
+        wf = WorkflowParser.parse(wf_data=wf_data, skipped_steps=["two"])
+        self.assertEqual(2, len(wf.steps))
+        self.assertEqual("one", wf.steps[0].id)
+        self.assertEqual("three", wf.steps[1].id)
+
+        # more than one
+        wf = WorkflowParser.parse(wf_data=wf_data, skipped_steps=["one", "three"])
+        self.assertEqual(1, len(wf.steps))
+        self.assertEqual("two", wf.steps[0].id)
 
-# TODO add tests for:
-# - test_add_missing_ids
-# - test_apply_substitutions
-# - test_skip_tests
-# - test_filter_step
+        # non-existing name
+        self.assertRaises(
+            SystemExit,
+            WorkflowParser.parse,
+            **{"wf_data": wf_data, "skipped_steps": ["four"]}
+        )
+
+    def test_add_missing_ids(self):
+        wf_data = {"steps": [{"uses": "foo"}, {"uses": "bar"}]}
+        # skip one step
+        wf = WorkflowParser.parse(wf_data=wf_data)
+        self.assertEqual("1", wf.steps[0].id)
+        self.assertEqual("2", wf.steps[1].id)
+
+    def test_substitutions(self):
+        # test wrong format for substitution key
+        wf_data = {"steps": [{"uses": "whatever"}]}
+        self.assertRaises(
+            SystemExit,
+            WorkflowParser.parse,
+            **{"wf_data": wf_data, "substitutions": ["SUB1=WRONG"]}
+        )
+
+        # expect error when not all given subs are used
+        wf_data = {
+            "steps": [
+                {
+                    "uses": "some_$_SUB1",
+                    "id": "some other $_SUB2",
+                    "env": {"FOO": "env_$_SUB3"},
+                    "secrets": ["secret_$_SUB4"],
+                }
+            ]
+        }
+        substitutions = [
+            "_SUB1=ONE",
+            "_SUB2=TWO",
+            "_SUB3=THREE",
+            "_SUB4=4",
+            "_SUB5=UNUSED",
+        ]
+        self.assertRaises(
+            SystemExit,
+            WorkflowParser.parse,
+            **{"wf_data": wf_data, "substitutions": substitutions}
+        )
+
+        # allow loose substitutions
+        wf = WorkflowParser.parse(
+            wf_data=wf_data, substitutions=substitutions, allow_loose=True
+        )
+        step = wf.steps[0]
+        self.assertEqual("some_ONE", step.uses)
+        self.assertEqual("some other TWO", step.id)
+        self.assertEqual("env_THREE", step.env["FOO"])
+        self.assertEqual(("secret_4",), step.secrets)