Skip to content

Add revert_previous_change_set method to the Stack class #67

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.

Sign up for GitHub

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

Open
PiotrKozlowski wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add revert_previous_change_set method to the Stack class #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
33df020
Add revert_previous_change_set method to the Stack class
May 15, 2020
9339a9f
Fix broken test and rename method to reverts_to_previous_change_set
May 25, 2020
2393ba9
Update name of a log
Jun 1, 2020
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
14 changes: 14 additions & 0 deletions lib/openstax/aws/stack.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,8 @@ def create_change_set(options)
def apply_change_set(params: {}, wait: false)
logger.info("**** DRY RUN ****") if dry_run

@previous_parameters = deployed_parameters
Copy link
Copy Markdown
Member

@jpslav jpslav May 29, 2020

Choose a reason for hiding this comment

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

Do we need to persist the previous parameters or template somewhere? If the process dies, these instance variables will be lost and we will no longer be able to revert.

Copy link
Copy Markdown
Author

@PiotrKozlowski PiotrKozlowski Jun 1, 2020

Choose a reason for hiding this comment

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

Hmm, that's a good question. I'm not sure if this should be done in this or a in a separate issue. I also don't know what would be the best way to achive this - some temp file?
cc @TomWoodward


logger.info("Updating #{name} stack...")

params = parameters_for_update(overrides: params)
Expand Down Expand Up @@ -220,6 +222,18 @@ def apply_change_set(params: {}, wait: false)
change_set
end

def revert_to_previous_change_set(wait: false)
logger.info("**** DRY RUN ****") if dry_run

if @previous_parameters
logger.info("Reverting to previous change set...")
apply_change_set(params: @previous_parameters, wait: wait)
@previous_parameters = nil
else
Copy link
Copy Markdown
Member

@jpslav jpslav May 29, 2020

Choose a reason for hiding this comment

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

The previous change set could also have had a different stack template. So we'd need to use that too? (the change set we are trying to revert could be using both new parameter values and a new template). Code like https://github.com/openstax/aws-ruby/blob/master/lib/openstax/aws/stack.rb#L59-L60 is maybe helpful.

Copy link
Copy Markdown
Author

@PiotrKozlowski PiotrKozlowski Jun 10, 2020

Choose a reason for hiding this comment

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

Is this something which may happen only when we code this directly, like in the test: it "can be updated with a new template" where we create a new stack and then create new stack class and instead of calling create we just call apply_change_set?

In this case, it seems like it would be not possible to retrieve the first template data using cached @previous_template because it would be a new class.

It seems like this case for creating a new stack class and calling apply_change_set instead of create is something which would be outside of the current concept of reverting changes using @previous_parameters to store the previous values.

I was thinking about passing template_s3_url to the revert_to_previous_change_set and apply_change_set methods which would work but it would not update response for stack.template method

What do you think would be the best option to allow reverting templates?

Copy link
Copy Markdown
Member

@jpslav jpslav Jun 10, 2020

Choose a reason for hiding this comment

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

What about this? #69

Copy link
Copy Markdown
Author

@PiotrKozlowski PiotrKozlowski Jun 12, 2020

Choose a reason for hiding this comment

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

Looks good, I've added some fixes and updated cassettes: #71

logger.info("There are no saved previous parameters for #{name} stack.")
Copy link
Copy Markdown
Member

@jpslav jpslav May 29, 2020

Choose a reason for hiding this comment

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

Should this be an error? logger.error? And if an error should we exit or raise?

Copy link
Copy Markdown
Author

@PiotrKozlowski PiotrKozlowski Jun 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I would not call an error here and just let the user know that he called a method on a stack which does not have a previous version
If you want me to change that, please let me know what should happen in that case

end
end

def delete(wait: false)
logger.info("**** DRY RUN ****") if dry_run

Expand Down
Loading