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

harkylton · 2022-01-13T22:33:22Z

Resolves #723

Previously the update command would exit if any change sets status
was not equal to READY. However, when a stack does not contain
any updates, it will not be READY since there is nothing to execute.
This should not prevent other change sets to be executed.

To get around this, we introduce another change set status, NO_CHANGES,
and handle that gracefully.

To clean up the output a bit, we also pass on describing the changes
for empty change sets.

Continuation of #917. Thanks @henrist for the original PR and @jfalkenstein for the review:

Fix original PR feedback
Simplify code for skipping output for empty change set
Roll back optional delete, we should always clean up
Add test covering "change set" version of update command

PR Checklist

Wrote a good commit message & description [see guide below].
Commit message starts with [Resolve #issue-number].
Added/Updated unit tests.
Added/Updated integration tests (if applicable).
All unit tests (make test) are passing.
Used the same coding conventions as the rest of the project.
The new code passes pre-commit validations (pre-commit run --all-files).
The PR relates to only one subject with a clear title.
and description in grammatically correct, complete sentences.

Approver/Reviewer Checklist

Before merge squash related commits.

Other Information

Guide to writing a good commit

…le stacks Previously the update command would exit if any change sets status was not equal to READY. However, when a stack does not contain any updates, it will not be READY since there is nothing to execute. This should not prevent other change sets to be executed. To get around this, we introduce another change set status, NO_CHANGES, and handle that gracefully. To clean up the output a bit, we also pass on describing the changes for empty change sets.

jfalkenstein · 2022-01-13T23:06:22Z

sceptre/cli/update.py

-        except Exception as e:
-            raise e


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

jfalkenstein · 2022-01-13T23:07:42Z

Thanks for posting this. I'll take a look at this tomorrow morning, hopefully.

jfalkenstein · 2022-01-14T15:02:36Z

sceptre/cli/update.py

-            # Exit if change set fails to create
+
+            at_least_one_ready = False
+
            for status in list(statuses.values()):


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():

jfalkenstein · 2022-01-14T15:09:11Z

tests/test_cli.py

+        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


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).

I have split the tests into 3 test cases.

jfalkenstein · 2022-01-14T15:10:39Z

sceptre/cli/update.py

                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
+
        finally:
            # Clean up by deleting change set
            plan.delete_change_set(change_set_name)


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.

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.

jfalkenstein

This is a great start and I like where this is going. Thank you for contributing!

My feedback I have is mostly around maintainability improvements. If you haven't joined the Sceptre slack channel you can do it here and find the "sceptre" channel: http://slackhatesthe.cloud/. I'm usually around during my work hours (9am-5pm UTC-6), if you wanted a more immediate way to ask some questions, if any of my comments didn't make sense to you.

jfalkenstein · 2022-07-22T15:16:34Z

Hey @harkylton, just following up on this: Are you going to address the comments on this PR? What's the status on this work?

jfalkenstein · 2023-01-03T19:47:11Z

@harkylton, what is the status on this PR? Are you intending to pick this work back up? Or should we consider this work as abandoned?

mreeves1 · 2023-01-04T02:22:14Z

I am still hitting this issue and it's pretty annoying. IMO the value of sceptre is being able to check if a bunch of stacks need updating and apply them all if so. Instead I need to run a "global" diff and then individually run update -c.

I imagine merge conflict resolutions will be needed too.
I am not amazing at python but would be willing to try to address any PR criticisms.

jfalkenstein · 2023-01-04T15:39:40Z

@mreeves1, I'd be happy to consult to help you get it over the line, if you'd like that. If you haven't yet joined our slack channel, you should.

harkylton · 2023-01-04T20:42:39Z

@jfalkenstein @mreeves1 Hey guys, sorry for the inactivity! I've been on parental leave since this summer and now very busy back at work. I did start addressing some of this locally. I'll look it over tomorrow and push what I have to see if we can move this along

mreeves1 · 2023-01-04T21:00:39Z

@harkylton Congratulations! No worries. Let me know if there is anything I can do to help and best of health and happiness to you and your growing family in 2023.

alex-harvey-z3q · 2024-06-14T09:24:24Z

@mreeves1 @harkylton Are either of you planning on finishing this PR?

alex-harvey-z3q · 2024-06-23T04:42:00Z

Closing in favour of #1480

harkylton mentioned this pull request Jan 13, 2022

Fix update command with change sets for multiple stacks #917

Closed

9 tasks

jfalkenstein reviewed Jan 13, 2022

View reviewed changes

jfalkenstein reviewed Jan 14, 2022

View reviewed changes

jfalkenstein requested changes Jan 14, 2022

View reviewed changes

alex-harvey-z3q mentioned this pull request Jun 23, 2024

[Resolve #723] Fix update command with change sets for multiple stacks #1480

Merged

9 tasks

alex-harvey-z3q closed this Jun 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

harkylton commented Jan 13, 2022 •

edited

Loading

jfalkenstein Jan 13, 2022

jfalkenstein commented Jan 13, 2022

jfalkenstein Jan 14, 2022

alex-harvey-z3q Jun 23, 2024

jfalkenstein Jan 14, 2022

alex-harvey-z3q Jun 23, 2024

jfalkenstein Jan 14, 2022

alex-harvey-z3q Jun 23, 2024

jfalkenstein left a comment

jfalkenstein commented Jul 22, 2022

jfalkenstein commented Jan 3, 2023

mreeves1 commented Jan 4, 2023

jfalkenstein commented Jan 4, 2023

harkylton commented Jan 4, 2023

mreeves1 commented Jan 4, 2023

alex-harvey-z3q commented Jun 14, 2024

alex-harvey-z3q commented Jun 23, 2024

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

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

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

Conversation

harkylton commented Jan 13, 2022 • edited Loading

PR Checklist

Approver/Reviewer Checklist

Other Information

jfalkenstein Jan 13, 2022

Choose a reason for hiding this comment

jfalkenstein commented Jan 13, 2022

jfalkenstein Jan 14, 2022

Choose a reason for hiding this comment

alex-harvey-z3q Jun 23, 2024

Choose a reason for hiding this comment

jfalkenstein Jan 14, 2022

Choose a reason for hiding this comment

alex-harvey-z3q Jun 23, 2024

Choose a reason for hiding this comment

jfalkenstein Jan 14, 2022

Choose a reason for hiding this comment

alex-harvey-z3q Jun 23, 2024

Choose a reason for hiding this comment

jfalkenstein left a comment

Choose a reason for hiding this comment

jfalkenstein commented Jul 22, 2022

jfalkenstein commented Jan 3, 2023

mreeves1 commented Jan 4, 2023

jfalkenstein commented Jan 4, 2023

harkylton commented Jan 4, 2023

mreeves1 commented Jan 4, 2023

alex-harvey-z3q commented Jun 14, 2024

alex-harvey-z3q commented Jun 23, 2024

harkylton commented Jan 13, 2022 •

edited

Loading