[Resolves #723] Fix update command with change sets for multiple stacks #1192
Conversation
…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.
| except Exception as e: | ||
| raise e |
There was a problem hiding this comment.
Thank you! I hate these shenanigans in code. If you're not going to handle an error, don't pretend like you are!
|
Thanks for posting this. I'll take a look at this tomorrow morning, hopefully. |
| # Exit if change set fails to create | ||
|
|
||
| at_least_one_ready = False | ||
|
|
||
| for status in list(statuses.values()): |
There was a problem hiding this comment.
No need to cast the values to a list if you're iterating over them.
| for status in list(statuses.values()): | |
| for status in statuses.values(): |
| 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 |
There was a problem hiding this comment.
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).
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Hey @harkylton, just following up on this: Are you going to address the comments on this PR? What's the status on this work? |
|
@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? |
|
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 I imagine merge conflict resolutions will be needed too. |
|
@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. |
|
@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 |
|
@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. |
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:
PR Checklist
[Resolve #issue-number].make test) are passing.pre-commit run --all-files).and description in grammatically correct, complete sentences.
Approver/Reviewer Checklist
Other Information
Guide to writing a good commit