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

Issue #157. implement CloudFormation RollbackTriggers. #188

Merged
merged 3 commits into from
Apr 12, 2018

Conversation

taraspos
Copy link
Contributor

This PR implements RollbackTriggers for CloudFormation. Issue #157

Feature announcment
API Documentation

Tags []*string `awsName:"Tags" awsType:"awstagslice" templateName:"tags"`
PolicyBody *string `awsName:"StackPolicyBody" awsType:"awsstr"`
StackFile *string `templateName:"stack-file"`
RollbackConfigurations *cloudformation.RollbackConfiguration `awsName:"RollbackConfiguration" awsType:"awsrollbackconfig"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you try to remove the awsrollbackconfig definition entirely, meaning here in the metadata (awsType:"awsrollbackconfig") and in the setters.go file above (more playing the role of a stub I guess)?

I am not sure it is needed during the runtime but it should not be according to what you do with the RollbackConfigurations field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, unfortunately then RollbackConfigurations not being passed to the Update request payload.

@simcap
Copy link
Contributor

simcap commented Feb 1, 2018

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?

"policy-file": "The path to the file containing the stack policy body",
"template-file": "The path to the file containing the template body with a minimum size of 1 byte and a maximum size of 51,200 bytes",
"stack-file": "The path to the file containing Parameters/Tags/StackPolices definition (http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/continuous-delivery-codepipeline-cfn-artifacts.html#w2ab2c13c15c15). Values passed via CLI has higher priority than ones defined in StackFile",
"rollback-triggers": "List of ClodwatchAlarm Arns to monitor during and after creation",
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo on Cloudwatch Alarm

@taraspos
Copy link
Contributor Author

taraspos commented Feb 1, 2018

@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
Copy link
Contributor

simcap commented Apr 10, 2018

@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
Copy link
Contributor

simcap commented Apr 10, 2018

@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 e626663 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
Copy link
Contributor

simcap commented Apr 12, 2018

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

@simcap simcap merged commit 3afa24c into wallix:master Apr 12, 2018
simcap added a commit that referenced this pull request Apr 12, 2018
from template at the injection step instead of the
BeforeRun step.
See #188
@taraspos
Copy link
Contributor Author

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

@simcap
Copy link
Contributor

simcap commented Apr 12, 2018

@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
Copy link
Contributor Author

@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
Copy link
Contributor

simcap commented Apr 12, 2018

@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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants