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

provider/aws: ASG Notifications Resource #2197

Merged
merged 4 commits into from
Jun 5, 2015
Merged

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Jun 4, 2015

Opening this as a proposal for a new top level resource, to implement #1419

Syntax:

resource "aws_autoscaling_group" "asg_example" {
  name = "asg_with_notifications"
  [...]
}

resource "aws_autoscaling_group" "asg_example_2" {
  name = "other_asg_with_notifications"
  [...]
}

resource "aws_sns_topic" "some_topic" {
  name = "some_topic_name"
}

resource "aws_autoscaling_notification" "example" {
  group_name     = [
    "${aws_autoscaling_group.asg_example.name}",
    "${aws_autoscaling_group.asg_example_2.name}",
  ]
  notifications  = [
    "autoscaling:EC2_INSTANCE_LAUNCH", 
    "autoscaling:EC2_INSTANCE_TERMINATE"
  ]
  topic_arn      = "${aws_sns_topic.some_topic.arn}"
}

Motivation(s):

ASGs can send notifications to SNS Topics as events occur (see Notification Types below). The API supports a 1-1 relationship per ASG and SNS Topic.

I'm proposing a new top level resource primarily to support modulaization, such that a module author can yield an ASG and a consumer can then add notifications as needed.

Relevant APIs:

TODO:

  • implementation
  • acceptance tests
  • docs

Future:

The current proposal above only supports a 1-1 for ASG-Topic, but we could likely expand it to support multiple ASG names in the future, so that the same notification config is shared on multiple ASGs. A group_names attribute could be added, containing a list of ASG names to apply this configuration to.

@phinze
Copy link
Contributor

phinze commented Jun 2, 2015

LGTM - implement away! 🚀

@catsby
Copy link
Contributor Author

catsby commented Jun 4, 2015

Basic implementation is done -- need to update some things and make sure the _update test works and is meaningful, but wanted to push what I have

@stack72
Copy link
Contributor

stack72 commented Jun 5, 2015

@catsby this looks good. I guess I didn't like the idea of separation before but now I see the separation looks good :)

@catsby
Copy link
Contributor Author

catsby commented Jun 5, 2015

Implemented, tested, documented. Please review and test out. Thanks!

@stack72
Copy link
Contributor

stack72 commented Jun 5, 2015

@catsby nice work :) Hoping to see a release of this soon!

}

return nl
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we have this code elsewhere, but I'm unable to find it at the moment. Seems like a thing we should promote somewhere shared if it's common enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice on schema.Set.toList() or similar but this one is AWS-specific.

@phinze
Copy link
Contributor

phinze commented Jun 5, 2015

One nit that can be easily addressed port-merge - LGTM!

@mzupan
Copy link
Contributor

mzupan commented Jun 5, 2015

worked great 👍

Your example in the body of the PR is group_names = [] but the docs look ok

@phinze phinze changed the title provider/aws: Proposal for ASG Notifications Resource provider/aws: ASG Notifications Resource Jun 5, 2015

# aws\_vpc

Provides an AutoScaling Group with Notification support, via SNS Topics
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly this feature actually abstracts creating multiple AWS resources at the same time from the same configuration "template". We should note that here to make it clear that this does not map 1:1 to what you'd see the API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's tricky to describe what this creates... they are resources in a since, but you can't access them directly... anyway, your point is valid. I'll expand on that.

@cbednarski
Copy link
Contributor

@catsby LGTM!

catsby added a commit that referenced this pull request Jun 5, 2015
provider/aws: ASG Notifications Resource
@catsby catsby merged commit e172508 into master Jun 5, 2015
@catsby catsby deleted the f-aws-asg-notifications branch June 5, 2015 21:04
@catsby
Copy link
Contributor Author

catsby commented Jun 5, 2015

good catch @mzupan , I updated the comment, it's [] (multiple) not "" (single)

@ghost
Copy link

ghost commented May 2, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators May 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants