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

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

Merged
merged 1 commit into from Oct 18, 2017

Conversation

Projects
None yet
3 participants
@cboss24
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.
@theseanything

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.

Show outdated Hide outdated docs/docs/stack_config.md
@cboss24

This comment has been minimized.

Show comment
Hide comment
@cboss24

cboss24 Oct 18, 2017

Contributor

@seanrankine first round of PR comments addressed.

Contributor

cboss24 commented Oct 18, 2017

@seanrankine first round of PR comments addressed.

@theseanything

theseanything suggested 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.

@cboss24

This comment has been minimized.

Show comment
Hide comment
@cboss24

cboss24 Oct 18, 2017

Contributor

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

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?

@b-t-g

This comment has been minimized.

Show comment
Hide comment
@b-t-g

b-t-g Oct 18, 2017

Contributor

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

Contributor

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

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

This comment has been minimized.

Show comment
Hide comment
@cboss24

cboss24 Oct 18, 2017

Contributor

@seanrankine squashed!

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

JuanCanham added a commit to JuanCanham/sceptre that referenced this pull request Jun 14, 2018

[Resolve #184] Add support for SNS stack notifications (#220)
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment