Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Resolves #723] Fix update command with change sets for multiple stacks #1192

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions sceptre/cli/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,23 +57,38 @@ def update_command(ctx, path, change_set, verbose, yes):
try:
# Wait for change set to be created
statuses = plan.wait_for_cs_completion(change_set_name)
# Exit if change set fails to create

at_least_one_ready = False

for status in list(statuses.values()):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to cast the values to a list if you're iterating over them.

Suggested change
for status in list(statuses.values()):
for status in statuses.values():

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if status != StackChangeSetStatus.READY:
# Exit if change set fails to create
if status not in (StackChangeSetStatus.READY, StackChangeSetStatus.NO_CHANGES):
write("Failed to create change set", context.output_format)
exit(1)

if status == StackChangeSetStatus.READY:
at_least_one_ready = True

# If none are ready, and we haven't exited, there are no changes
if not at_least_one_ready:
write("No changes detected", context.output_format)
exit(0)

# Describe changes
descriptions = plan.describe_change_set(change_set_name)
for description in list(descriptions.values()):
for stack, description in descriptions.items():
# No need to print if there are no changes
if statuses[stack] == StackChangeSetStatus.NO_CHANGES:
continue

if not verbose:
description = simplify_change_set_description(description)
write(description, context.output_format)

# Execute change set if happy with changes
if yes or click.confirm("Proceed with stack update?"):
plan.execute_change_set(change_set_name)
except Exception as e:
raise e
Comment on lines -75 to -76
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I hate these shenanigans in code. If you're not going to handle an error, don't pretend like you are!


finally:
# Clean up by deleting change set
plan.delete_change_set(change_set_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 55-94 is a very large segment of code. You could break all or most of that into outside functions that could be independently unit tested. It would make the code easier to test, easier to maintain, and easier to read.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree although I would note that the original PR is fairly consistent stylistically with the rest of the code in the CLI and without Jon here to say exactly what he had in mind, I vote for leaving this section alone for now.

Expand Down
7 changes: 7 additions & 0 deletions sceptre/plan/actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -884,6 +884,7 @@ def _get_cs_status(self, change_set_name):
cs_description = self.describe_change_set(change_set_name)

cs_status = cs_description["Status"]
cs_reason = cs_description.get("StatusReason")
cs_exec_status = cs_description["ExecutionStatus"]
possible_statuses = [
"CREATE_PENDING", "CREATE_IN_PROGRESS",
Expand Down Expand Up @@ -915,6 +916,12 @@ def _get_cs_status(self, change_set_name):
cs_exec_status in ["UNAVAILABLE", "AVAILABLE"]
):
return StackChangeSetStatus.PENDING
elif (
cs_status == "FAILED" and
cs_reason is not None and
self.change_set_creation_failed_due_to_no_changes(cs_reason)
):
return StackChangeSetStatus.NO_CHANGES
elif (
cs_status in ["DELETE_COMPLETE", "FAILED"] or
cs_exec_status in [
Expand Down
1 change: 1 addition & 0 deletions sceptre/stack_status.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,4 @@ class StackChangeSetStatus(object):
PENDING = "pending"
READY = "ready"
DEFUNCT = "defunct"
NO_CHANGES = "no changes"
11 changes: 11 additions & 0 deletions tests/test_actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -1088,6 +1088,17 @@ def test_get_cs_status_handles_all_statuses(
)
assert response == returns[i]

mock_describe_change_set.return_value = {
"Status": "FAILED",
"StatusReason": "The submitted information didn't contain changes. "
"Submit different information to create a change set.",
"ExecutionStatus": "UNAVAILABLE"
}
response = self.actions._get_cs_status(
sentinel.change_set_name
)
assert response == scss.NO_CHANGES

for status in return_values['Status']:
mock_describe_change_set.return_value = {
"Status": status,
Expand Down
88 changes: 87 additions & 1 deletion tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
from sceptre.exceptions import SceptreException
from sceptre.plan.actions import StackActions
from sceptre.stack import Stack
from sceptre.stack_status import StackStatus
from sceptre.stack_status import StackChangeSetStatus, StackStatus


class TestCli(object):
Expand Down Expand Up @@ -484,6 +484,92 @@ def test_stack_commands(self, command, success, yes_flag, exit_code):
run_command.assert_called_with()
assert result.exit_code == exit_code

@pytest.mark.parametrize(
"change_set_status,yes_flag,exit_code,verbose_flag", [
(StackChangeSetStatus.READY, True, 0, True),
(StackChangeSetStatus.READY, True, 0, False),
(StackChangeSetStatus.READY, False, 0, True),
(StackChangeSetStatus.READY, False, 0, False),
(StackChangeSetStatus.NO_CHANGES, True, 0, True),
(StackChangeSetStatus.NO_CHANGES, False, 0, False),
(StackChangeSetStatus.NO_CHANGES, True, 0, False),
(StackChangeSetStatus.NO_CHANGES, False, 0, True),
(StackChangeSetStatus.DEFUNCT, True, 1, True),
(StackChangeSetStatus.DEFUNCT, False, 1, False),
(StackChangeSetStatus.DEFUNCT, True, 1, False),
(StackChangeSetStatus.DEFUNCT, False, 1, True),
]
)
def test_update_with_change_set(self, change_set_status, yes_flag, exit_code, verbose_flag):
create_command = self.mock_stack_actions.create_change_set
wait_command = self.mock_stack_actions.wait_for_cs_completion
execute_command = self.mock_stack_actions.execute_change_set
delete_command = self.mock_stack_actions.delete_change_set
describe_command = self.mock_stack_actions.describe_change_set

wait_command.return_value = change_set_status

response = {
"VerboseProperty": "VerboseProperty",
"ChangeSetName": "ChangeSetName",
"CreationTime": "CreationTime",
"ExecutionStatus": "ExecutionStatus",
"StackName": "StackName",
"Status": "Status",
"StatusReason": "StatusReason",
"Changes": [
{
"ResourceChange": {
"Action": "Action",
"LogicalResourceId": "LogicalResourceId",
"PhysicalResourceId": "PhysicalResourceId",
"Replacement": "Replacement",
"ResourceType": "ResourceType",
"Scope": "Scope",
"VerboseProperty": "VerboseProperty"
}
}
]
}

if not verbose_flag:
del response["VerboseProperty"]
del response["Changes"][0]["ResourceChange"]["VerboseProperty"]

describe_command.return_value = response

kwargs = {"args": ["update", "--change-set", "dev/vpc.yaml"]}

if yes_flag:
kwargs["args"].append("-y")
else:
kwargs["input"] = "y\n"

if verbose_flag:
kwargs["args"].append("-v")

result = self.runner.invoke(cli, **kwargs)

change_set_name = create_command.call_args[0][0]
assert 'change-set' in change_set_name

assert wait_command.called_with(change_set_name)
assert delete_command.called_with(change_set_name)

if change_set_status == StackChangeSetStatus.READY:
assert execute_command.called_with(change_set_name)
assert describe_command.called_with(change_set_name)
output = result.output.splitlines()[0]
assert yaml.safe_load(output) == response

if change_set_status == StackChangeSetStatus.DEFUNCT:
assert "Failed to create change set" in result.output

if change_set_status == StackChangeSetStatus.NO_CHANGES:
assert "No changes detected" in result.output

assert result.exit_code == exit_code
Comment on lines +535 to +571
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you have so much conditional logic in a parameterized test, it's better to just make multiple tests, one for each test case you want to test. This is a test trying to be a bunch of different tests at the same time. I think you should break this up so you don't need all this conditional logic in the same test. (Note: I'm fully aware there are a lot of bad examples of unit tests in this file. But we shouldn't make it worse by perpetuating those bad patterns).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have split the tests into 3 test cases.


@pytest.mark.parametrize(
"command, ignore_dependencies", [
("create", True),
Expand Down