Repository: aws-quickstart/taskcat
Pull request #555 (2020-04-20 05:50:11)

Batched addition of arguments throughout.
## Overview

Brief description of what this PR does, and why it is needed (use case)?

This PR...
~~- adds `--output-directory` to `taskcat test run`~~
- adds `--profile` as a root-level parameter. (Ex: `taskcat --profile foo test run`)
- honors `package_lambda: false`
- Adds decorators that allow us to further customize the argument behavior. 

### Law of Unintended Consequences

Adding `--profile` as a root-level argument means that it's passed to every subbcommand module. For now, simply adding `*_args, **_kwargs` to each module class settles it. However, this means that each subcommand may behave differently. We may want to centralize a bare-bones config object (OR - args) to pass-through so that when a new subcommand is added, it's not missed. 

Would very much appreciate feedback on this caveat, as it directly impacts another PR that I have in the works.

### Decorators

Two decorators are added 
- `CliCore.dont_generate_parameter`
- `CliCore.longform_param_required`

These decorators allow us to turn on/off parameter generation, and enforce longform parameters. 

On/off example is `profile` in `taskcat._cli_modules.Test.run`.
```python
    @CliCore.longform_param_required('custom_uid')
    @CliCore.dont_generate_parameter('profile')
    def run(  # noqa: C901
        test_names: str = "ALL",
        regions: str = "ALL",
        input_file: str = "./.taskcat.yml",
        project_root: str = "./",
        no_delete: bool = False,
        lint_disable: bool = False,
        enable_sig_v2: bool = False,
        keep_failed: bool = False,
        output_directory: str = './taskcat_outputs',
        profile: str = "default"
    ):
```

It's passed from the global argparser instance, but generating another parameter conflicts because `-p/--project-root` conflicts with the default naming `-p/--profile`. Redefining this parameter isn't necessary, so we're passing it from the root-level parser, but not asking for it again. 

The longform enforcer is, well, face-value. There are certain parameters that we may want to enforce the long-form of them. 

- Ex: `taskcat test run --super-dangerous-action-yes-i-understand`. In the current behavior, the arguments accepted would be `-s/--super-dangerous-yes-i-understand`. 

## Testing/Steps taken to ensure quality

How did you validate the changes in this PR?
- Unit tests. 
- by hand. 


 How to test this PR Start after checking out this branch (bulleted)
 * Include test case, and expected output

andrew-glenn: /do-e2e-tests
andrew-glenn: /do-e2e-tests
andrew-glenn: /do-e2e-tests
andrew-glenn: Yeah, I think a better approach is necessary here. I’m concerned it’ll involve non-trivial modifications to `_cli_core.py`
andrew-glenn: /do-e2e-tests
andrew-glenn: /do-e2e-tests

Patch:
diff --git a/taskcat/_cli.py b/taskcat/_cli.py
index 32a6dbb..caa3c75 100644
--- a/taskcat/_cli.py
+++ b/taskcat/_cli.py
@@ -1,11 +1,10 @@
-import argparse
 import signal
 import sys
 
 import requests
 from pkg_resources import get_distribution
 
-from taskcat._cli_core import CliCore
+from taskcat._cli_core import GLOBAL_ARGS, CliCore, _get_log_level
 from taskcat._common_utils import exit_with_code
 from taskcat._logger import PrintMsg, init_taskcat_cli_logger
 from taskcat.exceptions import TaskCatException
@@ -19,12 +18,6 @@ BANNER = (
     "___/_|\\_\\___\\__,_|\\__|\n                                \n"
 )
 
-
-class SetVerbosity(argparse.Action):
-    def __call__(self, parser, namespace, values, option_string=None):
-        LOG.setLevel(_get_log_level([option_string]))
-
-
 NAME = "taskcat"
 DESCRIPTION = (
     "taskcat is a tool that tests AWS CloudFormation templates. It deploys "
@@ -34,26 +27,6 @@ DESCRIPTION = (
     "include in the test, and pass in parameter values from your AWS "
     "CloudFormation template."
 )
-GLOBAL_ARGS = [
-    [
-        ["-q", "--quiet"],
-        {
-            "action": SetVerbosity,
-            "nargs": 0,
-            "help": "reduce output to the minimum",
-            "dest": "_quiet",
-        },
-    ],
-    [
-        ["-d", "--debug"],
-        {
-            "action": SetVerbosity,
-            "nargs": 0,
-            "help": "adds debug output and tracebacks",
-            "dest": "_debug",
-        },
-    ],
-]
 
 
 def main(cli_core_class=CliCore, exit_func=exit_with_code):
@@ -65,8 +38,11 @@ def main(cli_core_class=CliCore, exit_func=exit_with_code):
     try:
         _welcome()
         version = get_installed_version()
-        cli = cli_core_class(NAME, _cli_modules, DESCRIPTION, version, GLOBAL_ARGS)
+        cli = cli_core_class(NAME, _cli_modules, DESCRIPTION, version, GLOBAL_ARGS.ARGS)
         cli.parse(args)
+        _default_profile = cli.parsed_args.__dict__.get("_profile")
+        if _default_profile:
+            GLOBAL_ARGS.profile = _default_profile
         cli.run()
     except TaskCatException as e:
         LOG.error(str(e), exc_info=_print_tracebacks(log_level))
@@ -88,17 +64,6 @@ def _print_tracebacks(log_level):
     return log_level == "DEBUG"
 
 
-def _get_log_level(args, exit_func=exit_with_code):
-    log_level = "INFO"
-    if ("-d" in args or "--debug" in args) and ("-q" in args or "--quiet" in args):
-        exit_func(1, "--debug and --quiet cannot be specified simultaneously")
-    if "-d" in args or "--debug" in args:
-        log_level = "DEBUG"
-    if "-q" in args or "--quiet" in args:
-        log_level = "ERROR"
-    return log_level
-
-
 def _print_upgrade_msg(new_version, version):
     LOG.info("version %s\n" % version, extra={"nametag": ""})
     LOG.warning("A newer version of %s is available (%s)", NAME, new_version)
diff --git a/taskcat/_cli_core.py b/taskcat/_cli_core.py
index 5d42667..b680380 100644
--- a/taskcat/_cli_core.py
+++ b/taskcat/_cli_core.py
@@ -7,13 +7,83 @@ import inspect
 import logging
 import sys
 import types
+from typing import List
+
+from taskcat._common_utils import exit_with_code
 
 LOG = logging.getLogger(__name__)
 
 
+def _get_log_level(args, exit_func=exit_with_code):
+    log_level = "INFO"
+    if ("-d" in args or "--debug" in args) and ("-q" in args or "--quiet" in args):
+        exit_func(1, "--debug and --quiet cannot be specified simultaneously")
+    if "-d" in args or "--debug" in args:
+        log_level = "DEBUG"
+    if "-q" in args or "--quiet" in args:
+        log_level = "ERROR"
+    return log_level
+
+
+class SetVerbosity(argparse.Action):
+    def __call__(self, parser, namespace, values, option_string=None):
+        LOG.setLevel(_get_log_level([option_string]))
+
+
+class GlobalArgs:
+    ARGS = [
+        [
+            ["-q", "--quiet"],
+            {
+                "action": SetVerbosity,
+                "nargs": 0,
+                "help": "reduce output to the minimum",
+                "dest": "_quiet",
+            },
+        ],
+        [
+            ["-d", "--debug"],
+            {
+                "action": SetVerbosity,
+                "nargs": 0,
+                "help": "adds debug output and tracebacks",
+                "dest": "_debug",
+            },
+        ],
+        [["--profile"], {"help": "set the default profile used.", "dest": "_profile"}],
+    ]
+
+    def __init__(self):
+        self._profile = "default"
+
+    @property
+    def profile(self):
+        return self._profile
+
+    @profile.setter
+    def profile(self, profile):
+        self._profile = profile
+
+
+GLOBAL_ARGS = GlobalArgs()
+
+
 class CliCore:
     USAGE = "{prog}{global_opts}{command}{command_opts}{subcommand}{subcommand_opts}"
 
+    longform_required: List = []
+
+    @classmethod
+    def longform_param_required(cls, param_name):
+        def wrapper(command_func):
+            formatted_param = param_name.lower().replace("_", "-")
+            cls.longform_required.append(
+                f"{command_func.__qualname__}.{formatted_param}"
+            )
+            return command_func
+
+        return wrapper
+
     def __init__(self, prog_name, module_package, description, version=None, args=None):
         self.name = prog_name
         self.module_package = module_package
@@ -39,8 +109,7 @@ class CliCore:
         methods = inspect.getmembers(module, predicate=inspect.isfunction)
         return [method for method in methods if not method[0].startswith("_")]
 
-    @staticmethod
-    def _get_params(item):
+    def _get_params(self, item):
         params = []
         for param in inspect.signature(item).parameters.values():
             if param.name == "self" or param.name.startswith("_"):
@@ -59,9 +128,13 @@ class CliCore:
                 )
             if action == "store":
                 kwargs.update({"type": val_type})
-            params.append(
-                [[name] if required else [f"-{name[0]}", f"--{name}"], kwargs]
-            )
+            if required:
+                params.append([[name], kwargs])
+            else:
+                if f"{item.__qualname__}.{name}" in self.longform_required:
+                    params.append([[f"--{name}"], kwargs])
+                else:
+                    params.append([[f"-{name[0]}", f"--{name}"], kwargs])
         return params
 
     @staticmethod


Test patch:
diff --git a/taskcat/_cli_modules/test.py b/taskcat/_cli_modules/test.py
index cbf66e5..4229a8a 100644
--- a/taskcat/_cli_modules/test.py
+++ b/taskcat/_cli_modules/test.py
@@ -10,6 +10,7 @@ import yaml
 from taskcat._cfn._log_stack_events import _CfnLogTools
 from taskcat._cfn.threaded import Stacker
 from taskcat._cfn_lint import Lint as TaskCatLint
+from taskcat._cli_core import GLOBAL_ARGS
 from taskcat._client_factory import Boto3Cache
 from taskcat._common_utils import determine_profile_for_region
 from taskcat._config import Config
@@ -115,7 +116,8 @@ class Test:
         """
         project_root_path: Path = Path(project_root).expanduser().resolve()
         input_file_path: Path = project_root_path / input_file
-        args = _build_args(enable_sig_v2, regions)
+        # pylint: disable=too-many-arguments
+        args = _build_args(enable_sig_v2, regions, GLOBAL_ARGS.profile)
         config = Config.create(
             project_root=project_root_path,
             project_config_path=input_file_path,
@@ -134,7 +136,8 @@ class Test:
             if errors or not lint.passed:
                 raise TaskCatException("Lint failed with errors")
         # 2. build lambdas
-        LambdaBuild(config, config.project_root)
+        if config.config.project.package_lambda:
+            LambdaBuild(config, project_root_path)
         # 3. s3 sync
         buckets = config.get_buckets(boto3_cache)
         stage_in_s3(buckets, config.config.project.name, config.project_root)
@@ -248,7 +251,7 @@ def _trim_tests(test_names, config):
                 del config.config.tests[test]
 
 
-def _build_args(enable_sig_v2, regions):
+def _build_args(enable_sig_v2, regions, default_profile):
     args: Dict[str, Any] = {}
     if enable_sig_v2:
         args["project"] = {"s3_enable_sig_v2": enable_sig_v2}
@@ -256,4 +259,10 @@ def _build_args(enable_sig_v2, regions):
         if "project" not in args:
             args["project"] = {}
         args["project"]["regions"] = regions.split(",")
+    if default_profile:
+        _auth_dict = {"default": default_profile}
+        if not args.get("project"):
+            args["project"] = {"auth": _auth_dict}
+        else:
+            args["project"]["auth"] = _auth_dict
     return args
diff --git a/tests/test_cli.py b/tests/test_cli.py
index 9bfe3ff..23b8d3c 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -15,7 +15,7 @@ class TestCli(unittest.TestCase):
     @mock.patch("taskcat._cli.LOG.error")
     @mock.patch("taskcat._cli._welcome", autospec=True)
     @mock.patch("taskcat._cli.get_installed_version", autospec=True)
-    @mock.patch("taskcat._cli_core.CliCore", autospec=True)
+    @mock.patch("taskcat._cli_core.CliCore")
     @mock.patch("taskcat._cli._setup_logging", autospec=True)
     @mock.patch("taskcat._common_utils.exit_with_code", autospec=True)
     @mock.patch("sys.argv", autospec=True)
@@ -23,6 +23,9 @@ class TestCli(unittest.TestCase):
     def test_main(
         self, m_signal, m_argv, m_exit, m_log_setup, m_cli, m_ver, m_welcome, m_error
     ):
+        mock_clicore_instantiation = mock.MagicMock()
+        mock_clicore_instantiation.parsed_args = mock.MagicMock()
+        m_cli.return_value = mock_clicore_instantiation
         main(cli_core_class=m_cli, exit_func=m_exit)
         self.assertEqual(True, m_signal.called)
         self.assertEqual(True, m_log_setup.called)
diff --git a/tests/test_cli_core.py b/tests/test_cli_core.py
index 3a87de5..e67f60c 100644
--- a/tests/test_cli_core.py
+++ b/tests/test_cli_core.py
@@ -2,14 +2,13 @@ import unittest
 
 import mock
 from taskcat import _cli_modules
-from taskcat._cli import GLOBAL_ARGS
-from taskcat._cli_core import CliCore
+from taskcat._cli_core import GLOBAL_ARGS, CliCore
 
 
 class TestCliCore(unittest.TestCase):
     def test_cli_core(self):
         cli = CliCore(
-            "taskcat-test", _cli_modules, "test description", "0.1", GLOBAL_ARGS
+            "taskcat-test", _cli_modules, "test description", "0.1", GLOBAL_ARGS.ARGS
         )
         self.assertIn("lint", cli._modules)
         self.assertIn("package", cli._modules)
@@ -22,7 +21,7 @@ class TestCliCore(unittest.TestCase):
 
     def test_parse(self):
         cli = CliCore(
-            "taskcat-test", _cli_modules, "test description", "0.1", GLOBAL_ARGS
+            "taskcat-test", _cli_modules, "test description", "0.1", GLOBAL_ARGS.ARGS
         )
         cli.parser.parse_args = mock.Mock()
         actual = cli.parse()
@@ -30,7 +29,7 @@ class TestCliCore(unittest.TestCase):
 
     def test_run(self):
         cli = CliCore(
-            "taskcat-test", _cli_modules, "test description", "0.1", GLOBAL_ARGS
+            "taskcat-test", _cli_modules, "test description", "0.1", GLOBAL_ARGS.ARGS
         )
         cli._modules["lint"] = mock.Mock()
         cli.parse(["lint", "-i", "test-taskcat.yml", "-p", "./"])