Repository: aws-cloudformation/cloudformation-cli-python-plugin

Issue:
No-docker option in the CLI missing
use-docker switch was done but corresponding "no-docker" switch is missing.

Discussion:

Patch:
diff --git a/python/rpdk/python/codegen.py b/python/rpdk/python/codegen.py
index f5c49cf..f2c1d30 100644
--- a/python/rpdk/python/codegen.py
+++ b/python/rpdk/python/codegen.py
@@ -55,12 +55,16 @@ class Python36LanguagePlugin(LanguagePlugin):
         self.package_name = None
         self.package_root = None
         self._use_docker = None
+        self._no_docker = None
         self._protocol_version = "2.0.0"
 
     def _init_from_project(self, project):
         self.namespace = tuple(s.lower() for s in project.type_info)
         self.package_name = "_".join(self.namespace)
-        self._use_docker = project.settings.get("use_docker")
+        # Check config file for (legacy) 'useDocker' and use_docker settings
+        self._use_docker = project.settings.get("useDocker") or project.settings.get(
+            "use_docker"
+        )
         self.package_root = project.root / "src"
 
     def _init_settings(self, project):
@@ -78,14 +82,29 @@ class Python36LanguagePlugin(LanguagePlugin):
                 ".resource", ".test_entrypoint"
             )
 
-        self._use_docker = self._use_docker or input_with_validation(
-            "Use docker for platform-independent packaging (Y/n)?\n",
-            validate_no,
-            "This is highly recommended unless you are experienced \n"
-            "with cross-platform Python packaging.",
-        )
+        # If use_docker specified in .rpdk-config file or cli switch
+        # Ensure only 1 is true, with preference to use_docker
+        if project.settings.get("use_docker") is True:
+            self._use_docker = True
+            self._no_docker = False
+        # If no_docker specified in .rpdk-config file or cli switch
+        elif project.settings.get("no_docker") is True:
+            self._use_docker = False
+            self._no_docker = True
+        else:
+            # If neither no_docker nor use_docker specified in .rpdk-config
+            # file or cli switch, prompt to use containers or not
+            self._use_docker = input_with_validation(
+                "Use docker for platform-independent packaging (Y/n)?\n",
+                validate_no,
+                "This is highly recommended unless you are experienced \n"
+                "with cross-platform Python packaging.",
+            )
+            self._no_docker = not self._use_docker
 
+        # project.settings will get saved into .rpdk-config by cloudformation-cli
         project.settings["use_docker"] = self._use_docker
+        project.settings["no_docker"] = self._no_docker
         project.settings["protocolVersion"] = self._protocol_version
 
     def init(self, project):
@@ -334,14 +353,24 @@ class Python36LanguagePlugin(LanguagePlugin):
 
         LOG.warning("Starting pip build.")
         try:
-            completed_proc = subprocess_run(  # nosec
-                command,
-                stdout=PIPE,
-                stderr=PIPE,
-                cwd=base_path,
-                check=True,
-                shell=True,
-            )
+            # On windows run pip command through the default shell (CMD)
+            if os.name == "nt":
+                completed_proc = subprocess_run(  # nosec
+                    command,
+                    stdout=PIPE,
+                    stderr=PIPE,
+                    cwd=base_path,
+                    check=True,
+                    shell=True,
+                )
+            else:
+                completed_proc = subprocess_run(  # nosec
+                    command,
+                    stdout=PIPE,
+                    stderr=PIPE,
+                    cwd=base_path,
+                    check=True,
+                )
             LOG.warning("pip build finished.")
         except (FileNotFoundError, CalledProcessError) as e:
             raise DownstreamError("pip build failed") from e
diff --git a/python/rpdk/python/parser.py b/python/rpdk/python/parser.py
index 518dbcb..04b55ff 100644
--- a/python/rpdk/python/parser.py
+++ b/python/rpdk/python/parser.py
@@ -9,7 +9,9 @@ def setup_subparser(subparsers, parents, python_version, python_version_number):
     )
     parser.set_defaults(language=python_version)
 
-    parser.add_argument(
+    group = parser.add_mutually_exclusive_group()
+
+    group.add_argument(
         "-d",
         "--use-docker",
         action="store_true",
@@ -18,6 +20,13 @@ def setup_subparser(subparsers, parents, python_version, python_version_number):
             with cross-platform Python packaging.""",
     )
 
+    group.add_argument(
+        "--no-docker",
+        action="store_true",
+        help="""Generally not recommended unless you are experienced
+            with cross-platform Python packaging""",
+    )
+
     return parser
 
 
diff --git a/src/cloudformation_cli_python_lib/resource.py b/src/cloudformation_cli_python_lib/resource.py
index bf7ba30..bfc9384 100644
--- a/src/cloudformation_cli_python_lib/resource.py
+++ b/src/cloudformation_cli_python_lib/resource.py
@@ -164,9 +164,13 @@ class Resource:
         try:
             return UnmodelledRequest(
                 clientRequestToken=request.bearerToken,
-                desiredResourceState=request.requestData.resourceProperties,
+                desiredResourceState=request.requestData.resourceProperties
+                if request.requestData.resourceProperties
+                else {},
                 previousResourceState=request.requestData.previousResourceProperties,
-                desiredResourceTags=request.requestData.stackTags,
+                desiredResourceTags=request.requestData.stackTags
+                if request.requestData.stackTags
+                else {},
                 previousResourceTags=request.requestData.previousStackTags,
                 systemTags=request.requestData.systemTags,
                 previousSystemTags=request.requestData.previousSystemTags,


Test patch:
diff --git a/tests/plugin/codegen_test.py b/tests/plugin/codegen_test.py
index a36d04b..fb6bce8 100644
--- a/tests/plugin/codegen_test.py
+++ b/tests/plugin/codegen_test.py
@@ -108,7 +108,32 @@ def resource_project(tmp_path):
         "rpdk.python.codegen.input_with_validation", autospec=True, side_effect=[False]
     )
     with patch_plugins, patch_wizard:
-        project.init(TYPE_NAME, PythonLanguagePlugin.NAME)
+        project.init(
+            TYPE_NAME,
+            PythonLanguagePlugin.NAME,
+            settings={"use_docker": False, "no_docker": True},
+        )
+    return project
+
+
+@pytest.fixture
+def resource_project_use_docker(tmp_path):
+    project = Project(root=tmp_path)
+
+    patch_plugins = patch.dict(
+        "rpdk.core.plugin_registry.PLUGIN_REGISTRY",
+        {PythonLanguagePlugin.NAME: lambda: PythonLanguagePlugin},
+        clear=True,
+    )
+    patch_wizard = patch(
+        "rpdk.python.codegen.input_with_validation", autospec=True, side_effect=[False]
+    )
+    with patch_plugins, patch_wizard:
+        project.init(
+            TYPE_NAME,
+            PythonLanguagePlugin.NAME,
+            settings={"use_docker": True, "no_docker": False},
+        )
     return project
 
 
@@ -125,7 +150,32 @@ def hook_project(tmp_path):
         "rpdk.python.codegen.input_with_validation", autospec=True, side_effect=[False]
     )
     with patch_plugins, patch_wizard:
-        project.init_hook(TYPE_NAME, PythonLanguagePlugin.NAME)
+        project.init_hook(
+            TYPE_NAME,
+            PythonLanguagePlugin.NAME,
+            settings={"use_docker": False, "no_docker": True},
+        )
+    return project
+
+
+@pytest.fixture
+def hook_project_use_docker(tmp_path):
+    project = Project(root=tmp_path)
+
+    patch_plugins = patch.dict(
+        "rpdk.core.plugin_registry.PLUGIN_REGISTRY",
+        {PythonLanguagePlugin.NAME: lambda: PythonLanguagePlugin},
+        clear=True,
+    )
+    patch_wizard = patch(
+        "rpdk.python.codegen.input_with_validation", autospec=True, side_effect=[False]
+    )
+    with patch_plugins, patch_wizard:
+        project.init_hook(
+            TYPE_NAME,
+            PythonLanguagePlugin.NAME,
+            settings={"use_docker": True, "no_docker": False},
+        )
     return project
 
 
@@ -172,6 +222,7 @@ def test__remove_build_artifacts_file_not_found(tmp_path):
 def test_initialize_resource(resource_project):
     assert resource_project.settings == {
         "use_docker": False,
+        "no_docker": True,
         "protocolVersion": "2.0.0",
     }
 
@@ -209,8 +260,53 @@ def test_initialize_resource(resource_project):
     ast.parse(files[f"{os.path.join('src', 'foo_bar_baz', 'handlers.py')}"].read_text())
 
 
+def test_initialize_resource_use_docker(resource_project_use_docker):
+    assert resource_project_use_docker.settings == {
+        "use_docker": True,
+        "no_docker": False,
+        "protocolVersion": "2.0.0",
+    }
+
+    files = get_files_in_project(resource_project_use_docker)
+    assert set(files) == {
+        ".gitignore",
+        ".rpdk-config",
+        "README.md",
+        "foo-bar-baz.json",
+        "requirements.txt",
+        f"{os.path.join('example_inputs', 'inputs_1_create.json')}",
+        f"{os.path.join('example_inputs', 'inputs_1_invalid.json')}",
+        f"{os.path.join('example_inputs', 'inputs_1_update.json')}",
+        "example_inputs",
+        "src",
+        f"{os.path.join('src', 'foo_bar_baz')}",
+        f"{os.path.join('src', 'foo_bar_baz', '__init__.py')}",
+        f"{os.path.join('src', 'foo_bar_baz', 'handlers.py')}",
+        "template.yml",
+    }
+
+    assert "__pycache__" in files[".gitignore"].read_text()
+    assert SUPPORT_LIB_NAME in files["requirements.txt"].read_text()
+
+    readme = files["README.md"].read_text()
+    assert resource_project_use_docker.type_name in readme
+    assert SUPPORT_LIB_PKG in readme
+    assert "handlers.py" in readme
+    assert "models.py" in readme
+
+    assert resource_project_use_docker.entrypoint in files["template.yml"].read_text()
+
+    # this is a rough check the generated Python code is valid as far as syntax
+    ast.parse(files[f"{os.path.join('src', 'foo_bar_baz', '__init__.py')}"].read_text())
+    ast.parse(files[f"{os.path.join('src', 'foo_bar_baz', 'handlers.py')}"].read_text())
+
+
 def test_initialize_hook(hook_project):
-    assert hook_project.settings == {"use_docker": False, "protocolVersion": "2.0.0"}
+    assert hook_project.settings == {
+        "use_docker": False,
+        "no_docker": True,
+        "protocolVersion": "2.0.0",
+    }
 
     files = get_files_in_project(hook_project)
     assert set(files) == {
@@ -242,6 +338,43 @@ def test_initialize_hook(hook_project):
     ast.parse(files[f"{os.path.join('src', 'foo_bar_baz', 'handlers.py')}"].read_text())
 
 
+def test_initialize_hook_use_docker(hook_project_use_docker):
+    assert hook_project_use_docker.settings == {
+        "use_docker": True,
+        "no_docker": False,
+        "protocolVersion": "2.0.0",
+    }
+
+    files = get_files_in_project(hook_project_use_docker)
+    assert set(files) == {
+        ".gitignore",
+        ".rpdk-config",
+        "README.md",
+        "foo-bar-baz.json",
+        "requirements.txt",
+        "src",
+        f"{os.path.join('src', 'foo_bar_baz')}",
+        f"{os.path.join('src', 'foo_bar_baz', '__init__.py')}",
+        f"{os.path.join('src', 'foo_bar_baz', 'handlers.py')}",
+        "template.yml",
+    }
+
+    assert "__pycache__" in files[".gitignore"].read_text()
+    assert SUPPORT_LIB_NAME in files["requirements.txt"].read_text()
+
+    readme = files["README.md"].read_text()
+    assert hook_project_use_docker.type_name in readme
+    assert SUPPORT_LIB_PKG in readme
+    assert "handlers.py" in readme
+    assert "models.py" in readme
+
+    assert hook_project_use_docker.entrypoint in files["template.yml"].read_text()
+
+    # this is a rough check the generated Python code is valid as far as syntax
+    ast.parse(files[f"{os.path.join('src', 'foo_bar_baz', '__init__.py')}"].read_text())
+    ast.parse(files[f"{os.path.join('src', 'foo_bar_baz', 'handlers.py')}"].read_text())
+
+
 def test_generate_resource(resource_project):
     resource_project.load_schema()
     before = get_files_in_project(resource_project)
@@ -406,6 +539,7 @@ def test__pip_build_called_process_error(tmp_path):
 
 def test__build_pip(plugin):
     plugin._use_docker = False
+    plugin._no_docker = True
 
     patch_pip = patch.object(plugin, "_pip_build", autospec=True)
     patch_docker = patch.object(plugin, "_docker_build", autospec=True)
@@ -416,8 +550,46 @@ def test__build_pip(plugin):
     mock_pip.assert_called_once_with(sentinel.base_path)
 
 
+def test__build_pip_posix(plugin):
+    patch_os_name = patch("rpdk.python.codegen.os.name", "posix")
+    patch_subproc = patch("rpdk.python.codegen.subprocess_run")
+
+    # Path must be set outside simulated os.name
+    temppath = Path(str(sentinel.base_path))
+    with patch_os_name, patch_subproc as mock_subproc:
+        plugin._pip_build(temppath)
+
+    mock_subproc.assert_called_once_with(
+        plugin._make_pip_command(temppath),
+        stdout=ANY,
+        stderr=ANY,
+        cwd=temppath,
+        check=ANY,
+    )
+
+
+def test__build_pip_windows(plugin):
+    patch_os_name = patch("rpdk.python.codegen.os.name", "nt")
+    patch_subproc = patch("rpdk.python.codegen.subprocess_run")
+
+    # Path must be set outside simulated os.name
+    temppath = Path(str(sentinel.base_path))
+    with patch_os_name, patch_subproc as mock_subproc:
+        plugin._pip_build(temppath)
+
+    mock_subproc.assert_called_once_with(
+        plugin._make_pip_command(temppath),
+        stdout=ANY,
+        stderr=ANY,
+        cwd=temppath,
+        check=ANY,
+        shell=True,
+    )
+
+
 def test__build_docker(plugin):
     plugin._use_docker = True
+    plugin._no_docker = False
 
     patch_pip = patch.object(plugin, "_pip_build", autospec=True)
     patch_docker = patch.object(plugin, "_docker_build", autospec=True)
@@ -431,6 +603,7 @@ def test__build_docker(plugin):
 # Test _build_docker on Linux/Unix-like systems
 def test__build_docker_posix(plugin):
     plugin._use_docker = True
+    plugin._no_docker = False
 
     patch_pip = patch.object(plugin, "_pip_build", autospec=True)
     patch_from_env = patch("rpdk.python.codegen.docker.from_env", autospec=True)
@@ -456,6 +629,7 @@ def test__build_docker_posix(plugin):
 # Test _build_docker on Windows
 def test__build_docker_windows(plugin):
     plugin._use_docker = True
+    plugin._no_docker = False
 
     patch_pip = patch.object(plugin, "_pip_build", autospec=True)
     patch_from_env = patch("rpdk.python.codegen.docker.from_env", autospec=True)
@@ -481,6 +655,7 @@ def test__build_docker_windows(plugin):
 # Test _build_docker if geteuid fails
 def test__build_docker_no_euid(plugin):
     plugin._use_docker = True
+    plugin._no_docker = False
 
     patch_pip = patch.object(plugin, "_pip_build", autospec=True)
     patch_from_env = patch("rpdk.python.codegen.docker.from_env", autospec=True)
diff --git a/tests/plugin/parser_test.py b/tests/plugin/parser_test.py
index cc18014..f29cdbf 100644
--- a/tests/plugin/parser_test.py
+++ b/tests/plugin/parser_test.py
@@ -1,3 +1,5 @@
+import pytest
+
 import argparse
 from rpdk.python.parser import (
     setup_subparser_python36,
@@ -13,10 +15,29 @@ def test_setup_subparser_python36():
 
     sub_parser = setup_subparser_python36(subparsers, [])
 
-    args = sub_parser.parse_args(["-d"])
+    args = sub_parser.parse_args([])
 
     assert args.language == "python36"
-    assert args.use_docker is True
+    assert args.use_docker is False
+    assert args.no_docker is False
+
+    short_args = sub_parser.parse_args(["-d"])
+    assert short_args.language == "python36"
+    assert short_args.use_docker is True
+    assert short_args.no_docker is False
+
+    long_args = sub_parser.parse_args(["--use-docker"])
+    assert long_args.language == "python36"
+    assert long_args.use_docker is True
+    assert long_args.no_docker is False
+
+    no_docker = sub_parser.parse_args(["--no-docker"])
+    assert no_docker.language == "python36"
+    assert no_docker.use_docker is False
+    assert no_docker.no_docker is True
+
+    with pytest.raises(SystemExit):
+        sub_parser.parse_args(["--no-docker", "--use-docker"])
 
 
 def test_setup_subparser_python37():
@@ -29,6 +50,25 @@ def test_setup_subparser_python37():
 
     assert args.language == "python37"
     assert args.use_docker is False
+    assert args.no_docker is False
+
+    short_args = sub_parser.parse_args(["-d"])
+    assert short_args.language == "python37"
+    assert short_args.use_docker is True
+    assert short_args.no_docker is False
+
+    long_args = sub_parser.parse_args(["--use-docker"])
+    assert long_args.language == "python37"
+    assert long_args.use_docker is True
+    assert long_args.no_docker is False
+
+    no_docker = sub_parser.parse_args(["--no-docker"])
+    assert no_docker.language == "python37"
+    assert no_docker.use_docker is False
+    assert no_docker.no_docker is True
+
+    with pytest.raises(SystemExit):
+        sub_parser.parse_args(["--no-docker", "--use-docker"])
 
 
 def test_setup_subparser_python38():
@@ -41,6 +81,25 @@ def test_setup_subparser_python38():
 
     assert args.language == "python38"
     assert args.use_docker is False
+    assert args.no_docker is False
+
+    short_args = sub_parser.parse_args(["-d"])
+    assert short_args.language == "python38"
+    assert short_args.use_docker is True
+    assert short_args.no_docker is False
+
+    long_args = sub_parser.parse_args(["--use-docker"])
+    assert long_args.language == "python38"
+    assert long_args.use_docker is True
+    assert long_args.no_docker is False
+
+    no_docker = sub_parser.parse_args(["--no-docker"])
+    assert no_docker.language == "python38"
+    assert no_docker.use_docker is False
+    assert no_docker.no_docker is True
+
+    with pytest.raises(SystemExit):
+        sub_parser.parse_args(["--no-docker", "--use-docker"])
 
 
 def test_setup_subparser_python39():
@@ -53,3 +112,22 @@ def test_setup_subparser_python39():
 
     assert args.language == "python39"
     assert args.use_docker is False
+    assert args.no_docker is False
+
+    short_args = sub_parser.parse_args(["-d"])
+    assert short_args.language == "python39"
+    assert short_args.use_docker is True
+    assert short_args.no_docker is False
+
+    long_args = sub_parser.parse_args(["--use-docker"])
+    assert long_args.language == "python39"
+    assert long_args.use_docker is True
+    assert long_args.no_docker is False
+
+    no_docker = sub_parser.parse_args(["--no-docker"])
+    assert no_docker.language == "python39"
+    assert no_docker.use_docker is False
+    assert no_docker.no_docker is True
+
+    with pytest.raises(SystemExit):
+        sub_parser.parse_args(["--no-docker", "--use-docker"])