Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[Resolve #184] Add support for SNS stack notifications #220

Merged
merged 1 commit into from Oct 18, 2017

Conversation

Projects
None yet
3 participants
Contributor

cboss24 commented Oct 16, 2017

Now able to specify a list of SNS topic ARNs within a stack config using the notifications field. Notifications for that stack will then be sent to the specified SNS topics. This is useful for monitoring and integrating with Cloudformation.

I took a first stab at writing integration tests for this, but it raised a few questions:

  • should the SNS topic be created ahead of time in the Sceptre integration account? Or should part of the setup for the integration tests be this topic creation.
  • I had some trouble testing my integration tests, mainly surrounding assuming the arn:aws:iam::458947373260:role/SceptreIntegrationTestsRole role. My work around was to change comment out this role in all the config.yaml files. Is this the correct work around?

Let me know what you think!


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Squashed related commits together after the changes have been approved.
  • Commit message starts with [Resolve #issue-number] (if a related issue exists).
  • Added unit tests.
  • Added 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 doesn't generate flake8 (make lint) offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.

Thanks! We can leave out the integration tests and unit tests would suffice. Reasons being it would require checking the SNS topic that notification was published and the AWS account for integration tests has a topic setup. A lot of effort to check functionality which I feel will be adequately checked by unit tests.

docs/docs/stack_config.md
@@ -117,6 +118,10 @@ A dictionary of [CloudFormation Tags](https://docs.aws.amazon.com/AWSCloudFormat
The ARN of a [CloudFormation Service Role](http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-iam-servicerole.html) that is assumed by CloudFormation to create, update or delete resources.
+### notifications
+
+List of SNS topic ARNs to publish stack related events to. A maximum of 5 ARNs can be specified per stack.
@theseanything

theseanything Oct 17, 2017

Contributor

Could we have a link to the AWS documentation?

@theseanything

theseanything Oct 17, 2017

Contributor

Could we also list which Sceptre actions (create, update, create-change-set) this parameter will affect?

@cboss24

cboss24 Oct 17, 2017

Contributor

These are the only links I can find, but they aren't great:

  1. http://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_CreateStack.html (would we have to add a link for create/update/delete?)
  2. http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/cfn-console-add-tags.html (kind of vague and not helpful)

Do you have a preference?

@theseanything

theseanything Oct 18, 2017

Contributor

Ah yeah, not great - and no page anchors either. I'd say first option - but more explicit in referencing the documentation e.g. More information about stack notifications can found under the relevant section in the AWS CloudFormation API documentation.

Contributor

cboss24 commented Oct 18, 2017

@seanrankine first round of PR comments addressed.

theseanything requested changes Oct 18, 2017 edited

Can we add a unit test to checks that notificationARNs is a empty list when not set in config? Also bring up to date with master.

Contributor

cboss24 commented Oct 18, 2017

@seanrankine second round of PR comments addressed. Let me know if this is ready to be squashed. Or would you prefer I squash after every PR round?

Member

b-t-g commented Oct 18, 2017

@cboss24, it can be done after the changes are approved. We mostly request squashing to keep the history neat and, if something needs to be reverted, it can be done so easily.

theseanything approved these changes Oct 18, 2017 edited

Thanks, looks good! Just need to squash those commits and then ready to merge.

[Resolve #184] Add support for SNS stack notifications
Now able to specify a list of SNS topic ARNs within a stack config
using the `notifications` field. Notifications for that stack will
then be sent to the specified SNS topics. This is useful for monitoring
and integrating with Cloudformation.
Contributor

cboss24 commented Oct 18, 2017

@seanrankine squashed!

@b-t-g b-t-g merged commit 24eb142 into cloudreach:master Oct 18, 2017

@cboss24 cboss24 deleted the cboss24:184-stack-notifications branch Oct 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment