Repository: onicagroup/runway

Issue:
[BUG] runway incorrectly shows parameters to be removed in "plan" 
### Bug Description

When running a cloudformation plan on an existing stack, the output states that parameters are to be removed:

```console
phil4079@L4CLYSQ2:~/runway-cloudformation-bug-test$ DEPLOY_ENVIRONMENT=common runway plan
[runway] could not find runway.variables.yml or runway.variables.yaml in the current directory; continuing without a variables file
[runway] deploy environment "common" is explicitly defined in the environment
[runway] if not correct, update the value or unset it to fall back to the name of the current git branch or parent directory
[runway] 
[runway] 
[runway] deployment_1:processing deployment (in progress)
[runway] deployment_1:processing regions sequentially...
[runway] 
[runway] deployment_1.runway-cfn-test.cfn:processing module in us-east-1 (in progress)
[runway] found environment file: /home/phil4079/runway-cloudformation-bug-test/runway-cfn-test.cfn/common.env
[runway] stack-definition.yaml:plan (in progress)
[runway] diffing stacks: runway-cfn-test-stack
[runway] philcorp-dev-runway-cfn-test-stack changes:
Parameters Removed: InstanceType, SSHLocation, WebServerCount
Replacements:
- Modify EC2SRTA14Y5Q (AWS::EC2::SubnetRouteTableAssociation)
- Modify PublicSubnet (AWS::EC2::Subnet)
Changes:
- Modify ASGPolicy (AWS::AutoScaling::ScalingPolicy)
- Modify PublicElasticLoadBalancer (AWS::ElasticLoadBalancing::LoadBalancer)
- Modify WebServerFleet (AWS::AutoScaling::AutoScalingGroup)
Show full change set? [y/n] n
[runway] RawTemplateBlueprint.get_output_definitions is deprecated and will be removed in a future release
[runway] runway-cfn-test-stack:complete
[runway] stack-definition.yaml:plan (complete)
[runway] deployment_1.runway-cfn-test.cfn:processing module in us-east-1 (complete)
[runway] deployment_1:processing deployment (complete)
```

However, when running the same stack in deploy mode, these parameters are NOT listed for removal (which is the correct behavior):

```console
phil4079@L4CLYSQ2:~/runway-cloudformation-bug-test$ DEPLOY_ENVIRONMENT=common runway deploy
[runway] could not find runway.variables.yml or runway.variables.yaml in the current directory; continuing without a variables file
[runway] deploy environment "common" is explicitly defined in the environment
[runway] if not correct, update the value or unset it to fall back to the name of the current git branch or parent directory
[runway] 
[runway] 
[runway] deployment_1:processing deployment (in progress)
[runway] deployment_1:processing regions sequentially...
[runway] 
[runway] deployment_1.runway-cfn-test.cfn:processing module in us-east-1 (in progress)
[runway] found environment file: /home/phil4079/runway-cloudformation-bug-test/runway-cfn-test.cfn/common.env
[runway] stack-definition.yaml:init (in progress)
[runway] cfngin_bucket cfngin-philcorp-dev-us-east-1 already exists
[runway] stack-definition.yaml:init (complete)
[runway] stack-definition.yaml:deploy (in progress)
[runway] philcorp-dev-runway-cfn-test-stack changes:
Replacements:
- Modify EC2SRTA14Y5Q (AWS::EC2::SubnetRouteTableAssociation)
- Modify PublicSubnet (AWS::EC2::Subnet)
Changes:
- Modify ASGPolicy (AWS::AutoScaling::ScalingPolicy)
- Modify PublicElasticLoadBalancer (AWS::ElasticLoadBalancing::LoadBalancer)
- Modify WebServerFleet (AWS::AutoScaling::AutoScalingGroup)
Execute the above changes? [y/n/v] n
[runway] runway-cfn-test-stack:skipped (canceled execution)

[runway] stack-definition.yaml:deploy (complete)
[runway] deployment_1.runway-cfn-test.cfn:processing module in us-east-1 (complete)
[runway] deployment_1:processing deployment (complete)
```

VIewing the change set in AWS console does not show any parameters to be removed (see attached).

[runway-cloudformation-bug-test-stack-20220128.txt](https://github.com/onicagroup/runway/files/7961947/runway-cloudformation-bug-test-stack-20220128.txt)

[runway-cloudformation-bug-test-change-set-20220128.txt](https://github.com/onicagroup/runway/files/7961967/runway-cloudformation-bug-test-change-set-20220128.txt)


### Expected Behavior

Both "plan" and "deploy" should not show any parameters being removed.

### Steps To Reproduce

Example project: ...

1.  Deploy cloudformation stack with runway
2.  Make a change to the stack
3.  Perform a runway "plan" and "deploy" to see the output discrepancy



### Runway version

2.4.4

### Installation Type

pypi (pip, pipenv, poetry, etc)

### OS / Environment

- OS:  Windows 10
- runway:  2.4.4

### Anything else?

_No response_

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/onicagroup/runway/pull/1245?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup) Report
> Merging [#1245](https://codecov.io/gh/onicagroup/runway/pull/1245?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup) (99c56ae) into [master](https://codecov.io/gh/onicagroup/runway/commit/2a41ab6982f8d0c927a0b256cf5c58e085fc5b0a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup) (2a41ab6) will **decrease** coverage by `0.04%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/onicagroup/runway/pull/1245/graphs/tree.svg?width=650&height=150&src=pr&token=Ku28I0RY80&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup)](https://codecov.io/gh/onicagroup/runway/pull/1245?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup)

```diff
@@            Coverage Diff             @@
##           master    #1245      +/-   ##
==========================================
- Coverage   89.46%   89.41%   -0.05%     
==========================================
  Files         247      247              
  Lines       15219    15223       +4     
==========================================
- Hits        13615    13612       -3     
- Misses       1604     1611       +7     
```


| [Impacted Files](https://codecov.io/gh/onicagroup/runway/pull/1245?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup) | Coverage Δ | |
|---|---|---|
| [runway/cfngin/actions/diff.py](https://codecov.io/gh/onicagroup/runway/pull/1245/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup#diff-cnVud2F5L2Nmbmdpbi9hY3Rpb25zL2RpZmYucHk=) | `81.48% <100.00%> (+0.56%)` | :arrow_up: |
| [runway/cfngin/providers/aws/default.py](https://codecov.io/gh/onicagroup/runway/pull/1245/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup#diff-cnVud2F5L2Nmbmdpbi9wcm92aWRlcnMvYXdzL2RlZmF1bHQucHk=) | `83.08% <100.00%> (ø)` | |
| [runway/cfngin/hooks/aws\_lambda.py](https://codecov.io/gh/onicagroup/runway/pull/1245/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup#diff-cnVud2F5L2Nmbmdpbi9ob29rcy9hd3NfbGFtYmRhLnB5) | `84.10% <0.00%> (-1.66%)` | :arrow_down: |
| [runway/env\_mgr/kbenv.py](https://codecov.io/gh/onicagroup/runway/pull/1245/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup#diff-cnVud2F5L2Vudl9tZ3Iva2JlbnYucHk=) | `78.76% <0.00%> (-0.69%)` | :arrow_down: |
| [runway/env\_mgr/tfenv.py](https://codecov.io/gh/onicagroup/runway/pull/1245/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup#diff-cnVud2F5L2Vudl9tZ3IvdGZlbnYucHk=) | `96.46% <0.00%> (-0.51%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/onicagroup/runway/pull/1245?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/onicagroup/runway/pull/1245?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup). Last update [2a41ab6...99c56ae](https://codecov.io/gh/onicagroup/runway/pull/1245?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup).


Patch:
diff --git a/runway/cfngin/actions/diff.py b/runway/cfngin/actions/diff.py
index 0e68b2f9..8604e9d0 100644
--- a/runway/cfngin/actions/diff.py
+++ b/runway/cfngin/actions/diff.py
@@ -210,9 +210,14 @@ class Action(deploy.Action):
 
         tags = deploy.build_stack_tags(stack)
 
+        try:
+            provider_stack = provider.get_stack(stack.fqn)
+        except exceptions.StackDoesNotExist:
+            provider_stack = None
+
         try:
             stack.resolve(self.context, provider)
-            parameters = self.build_parameters(stack)
+            parameters = self.build_parameters(stack, provider_stack)
             outputs = provider.get_stack_changes(
                 stack, self._template(stack.blueprint), parameters, tags
             )
diff --git a/runway/cfngin/providers/aws/default.py b/runway/cfngin/providers/aws/default.py
index 97b620a0..ab9b259d 100644
--- a/runway/cfngin/providers/aws/default.py
+++ b/runway/cfngin/providers/aws/default.py
@@ -1503,8 +1503,10 @@ class Provider(BaseProvider):
             # handling for orphaned changeset temp stacks
             if self.get_stack_status(stack_details) == self.REVIEW_STATUS:
                 raise exceptions.StackDoesNotExist(stack.fqn)
-            _old_template, old_params = self.get_stack_info(stack_details)
-            old_template: Dict[str, Any] = parse_cloudformation_template(_old_template)
+            old_template_raw, old_params = self.get_stack_info(stack_details)
+            old_template: Dict[str, Any] = parse_cloudformation_template(
+                old_template_raw
+            )
             change_type = "UPDATE"
         except exceptions.StackDoesNotExist:
             old_params: Dict[str, Union[List[str], str]] = {}


Test patch:
diff --git a/tests/unit/cfngin/actions/conftest.py b/tests/unit/cfngin/actions/conftest.py
new file mode 100644
index 00000000..56d46653
--- /dev/null
+++ b/tests/unit/cfngin/actions/conftest.py
@@ -0,0 +1,30 @@
+"""Pytest fixtures and plugins."""
+# pyright: basic
+from __future__ import annotations
+
+from datetime import datetime
+from typing import TYPE_CHECKING
+
+import pytest
+from mock import MagicMock
+
+from runway.cfngin.providers.aws.default import Provider
+
+if TYPE_CHECKING:
+    from mypy_boto3_cloudformation.type_defs import StackTypeDef
+    from pytest_mock import MockerFixture
+
+
+@pytest.fixture(scope="function")
+def provider_get_stack(mocker: MockerFixture) -> MagicMock:
+    """Patches ``runway.cfngin.providers.aws.default.Provider.get_stack``."""
+    return_value: StackTypeDef = {
+        "CreationTime": datetime(2015, 1, 1),
+        "Description": "something",
+        "Outputs": [],
+        "Parameters": [],
+        "StackId": "123",
+        "StackName": "foo",
+        "StackStatus": "CREATE_COMPLETE",
+    }
+    return mocker.patch.object(Provider, "get_stack", return_value=return_value)
diff --git a/tests/unit/cfngin/actions/test_diff.py b/tests/unit/cfngin/actions/test_diff.py
index d5ce5a65..6d72501e 100644
--- a/tests/unit/cfngin/actions/test_diff.py
+++ b/tests/unit/cfngin/actions/test_diff.py
@@ -10,7 +10,7 @@ from typing import TYPE_CHECKING, Optional
 
 import pytest
 from botocore.exceptions import ClientError
-from mock import MagicMock, patch
+from mock import MagicMock, Mock, patch
 
 from runway.cfngin.actions.diff import (
     Action,
@@ -18,13 +18,15 @@ from runway.cfngin.actions.diff import (
     diff_dictionaries,
     diff_parameters,
 )
+from runway.cfngin.exceptions import StackDoesNotExist
 from runway.cfngin.providers.aws.default import Provider
 from runway.cfngin.status import SkippedStatus
 
 from ..factories import MockProviderBuilder, MockThreadingEvent
 
 if TYPE_CHECKING:
-    from pytest import LogCaptureFixture, MonkeyPatch
+    from pytest import LogCaptureFixture
+    from pytest_mock import MockerFixture
 
     from ...factories import MockCFNginContext
 
@@ -82,11 +84,14 @@ class TestAction:
 
         assert action.bucket_name == bucket_name
 
-    def test_diff_stack_validationerror_template_too_large(
+    @pytest.mark.parametrize("stack_not_exist", [False, True])
+    def test__diff_stack_validationerror_template_too_large(
         self,
         caplog: LogCaptureFixture,
         cfngin_context: MockCFNginContext,
-        monkeypatch: MonkeyPatch,
+        mocker: MockerFixture,
+        provider_get_stack: MagicMock,
+        stack_not_exist: bool,
     ) -> None:
         """Test _diff_stack ValidationError - template too large."""
         caplog.set_level(logging.ERROR)
@@ -94,8 +99,12 @@ class TestAction:
         cfngin_context.add_stubber("cloudformation")
         cfngin_context.config.cfngin_bucket = ""
         expected = SkippedStatus("cfngin_bucket: existing bucket required")
-        provider = Provider(cfngin_context.get_session())  # type: ignore
-        mock_get_stack_changes = MagicMock(
+        mock_build_parameters = mocker.patch.object(
+            Action, "build_parameters", return_value=[]
+        )
+        mock_get_stack_changes = mocker.patch.object(
+            Provider,
+            "get_stack_changes",
             side_effect=ClientError(
                 {
                     "Error": {
@@ -104,22 +113,29 @@ class TestAction:
                     }
                 },
                 "create_change_set",
-            )
+            ),
+        )
+        provider = Provider(cfngin_context.get_session())  # type: ignore
+        stack = MagicMock(
+            blueprint=Mock(rendered="{}"),
+            fqn="test-stack",
+            locked=False,
+            region=cfngin_context.env.aws_region,
+            status=None,
         )
-        monkeypatch.setattr(provider, "get_stack_changes", mock_get_stack_changes)
-        stack = MagicMock()
-        stack.region = cfngin_context.env.aws_region
-        stack.name = "test-stack"
-        stack.fqn = "test-stack"
-        stack.blueprint.rendered = "{}"
-        stack.locked = False
-        stack.status = None
+        stack.name = "stack"
+
+        if stack_not_exist:
+            provider_get_stack.side_effect = StackDoesNotExist("test-stack")
 
         result = Action(
             context=cfngin_context,
             provider_builder=MockProviderBuilder(provider=provider),
             cancel=MockThreadingEvent(),  # type: ignore
         )._diff_stack(stack)
+        mock_build_parameters.assert_called_once_with(
+            stack, None if stack_not_exist else provider_get_stack.return_value
+        )
         mock_get_stack_changes.assert_called_once()
         assert result == expected