search

wallix / awless

A Mighty CLI for AWS
http://awless.io/
Apache License 2.0
4.97k stars 263 forks source link

Issue #157. implement CloudFormation RollbackTriggers. #188

Closed taraspos closed 6 years ago

taraspos commented 6 years ago

This PR implements RollbackTriggers for CloudFormation. Issue #157

Feature announcment API Documentation

simcap commented 6 years ago

Thanks for implementing this latest feature from AWS in awless!

I am a bit worried about the BeforeRun hook being the place where all the type processing is done since it should be done before using other means, but I guess this will do as for now. It is our fault though ;) as certain framework usage could be clearer.

Anyway have you tested that in real life and is it working well for you?

taraspos commented 6 years ago

@simcap I would be happy to fix the implementation if you point me in the right direction.

I thought it needs to be implemented via setters but I didn't see the proper way to use them to create cloudformation.RollbackConfiguration from 2 different parameters rollback-triggers and rollback-monitoring-min.

simcap commented 6 years ago

@Trane9991 Coming back on that, I might have a solution so that it will be more robust. Also I will write the corresponding acceptance test so that we know if we are on the right track.

I think I have write rights on this PR.

Let's try to make this PR and the other you just created part of the next release v0.1.10

simcap commented 6 years ago

@Trane9991 I wrote the acceptance test for the new params, so that at least we have a guarantee on how we want it to work regarding the implementation.

You can now cherry pick (or any other way), the following commit https://github.com/wallix/awless/commit/e62666300dfc7008d44a23d8a05030ac830fe189 and integrate it in this PR. We can then merge it to master I will subsequently code an implementation that do not do params injection in the before run step.

Thanks!

simcap commented 6 years ago

Simpler, I will merge it now and add the acceptance test myself.

taraspos commented 6 years ago

@simcap sorry for troubles, I was very busy last days and didn't have time to look here.

simcap commented 6 years ago

@Trane9991 No worries! I am side tracked as well on other stuff.

Also, if you want I can wait a bit so that your last PR can make it to the v0.1.10 that is due in the next few days. The PR was basically lacking a unit test on the main method I think ... if it was feasible.

taraspos commented 6 years ago

@simcap I will be able to work on it on weekends only. If you want to do release this week, go ahead and do not wait for me, or if it's not too troubling you can wait for me and do the release on Monday.

simcap commented 6 years ago

@Trane9991 Actually, I just remembered I am off next week. Since there a lot of improvements in the v.0.1.10 and it is long overdue, we will release before the week end.

But if you tell me that this PR has been well tested live on your terminal and that you are happy with it, I will merge it and you can add the unit test later on.

(If you do test a bit on your terminal a bit try to pull the latest master (which will included my update on rollback triggers.)

Let me know.

taraspos commented 6 years ago

@simcap it is tested, but not as well as I want it to.

Go ahead and release it without that PR, I will do more live testing on my CI workflow and we can put it into the v0.1.11.