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

Add additional Autoscaling and Cloudwatch resources #462

Closed
wants to merge 13 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@rgeddis
Copy link
Contributor

rgeddis commented Feb 29, 2016

New resources include:

  • New aws_cloudwatch_alarm resource
  • Add scaling_policies and notification_configurations to existing aws_autoscaling_policy resource
  • Documentation and integration testing for all the above

@rgeddis rgeddis changed the title Add additional AWS resources Add additional Autoscaling and Cloudwatch resources Feb 29, 2016

@joaogbcravo

This comment has been minimized.

Copy link
Contributor

joaogbcravo commented Mar 14, 2016

👍

@chasebolt

This comment has been minimized.

Copy link

chasebolt commented Mar 29, 2016

awesome! can't wait until this gets merged in 👍

statistic 'Average'
threshold 80
alarm_actions [
scaling_group.aws_object.scaling_policies['my-scaling-policy'].arn

This comment has been minimized.

@tyler-ball

tyler-ball Apr 27, 2016

Contributor

I'm thinking this needs a lazy wrapper around it because the aws_object won't exist during the first compile


def destroy_aws_object(_instance)
converge_by "destroying cloudwatch alarm #{new_resource.name} in #{region}" do
new_resource.driver.cloudwatch.alarms.delete(new_resource.name)

This comment has been minimized.

@tyler-ball

tyler-ball Apr 27, 2016

Contributor

Are the create and delete operations blocking? IE, will they wait to return until the cloudwatch alarm is created? Otherwise we need some waiters here

This comment has been minimized.

@rgeddis

rgeddis Apr 28, 2016

Author Contributor

As far I can tell from the AWS documentation and some local testing the create/delete are blocking operations, also the alarm specification doesn't have a status (for creating, available...etc states) in the same way the RDS resource does


aws_sdk_type AWS::CloudWatch::Alarm, id: :name

attribute :name, :kind_of => String, :name_attribute => true

This comment has been minimized.

@joaogbcravo

joaogbcravo Apr 28, 2016

Contributor

Would be nice, since this is a new provider, to respect the new "standard" of RFC 054:
https://github.com/chef/chef-rfc/blob/master/rfc054-resource-attribute-improvements.md

notification_configurations [{
topic: driver.build_arn(service: 'sns', resource: 'test_topic'),
types: [
'autoscaling:EC2_INSTANCE_LAUNCH',

This comment has been minimized.

@joaogbcravo

joaogbcravo Apr 28, 2016

Contributor

Thinking if the "types" should be validated somehow in compilation time.
If not in this PR, at least raise an feature issue, to make that later I guess. What do you think?

}]
scaling_policies(
test_policy: {
adjustment_type: 'ChangeInCapacity',

This comment has been minimized.

@joaogbcravo

joaogbcravo Apr 28, 2016

Contributor

Same thinking here. Should we have some kind of validation on the scaling policies adjustment types?

@joaogbcravo

This comment has been minimized.

Copy link
Contributor

joaogbcravo commented Apr 28, 2016

I'm ok with this pull request. Just raised some nice to have questions, but imo we can merge it @tyler-ball

@tyler-ball

This comment has been minimized.

Copy link
Contributor

tyler-ball commented Apr 29, 2016

@joaogbcravo If AWS gives good error messages I don't like to duplicate validation on our side that they will perform. I'll check and see if the error messages are good for your suggestions above!

tyler-ball added a commit that referenced this pull request May 2, 2016

Updating #462 and getting it ready for merge. Moved to the V2 SDK, ad…
…ded support for updating the resource and expanded testing.

tyler-ball added a commit that referenced this pull request May 2, 2016

Merge Pull Request
#462 from
cloudsmith-io/master
@tyler-ball

This comment has been minimized.

Copy link
Contributor

tyler-ball commented May 2, 2016

Merged in 7f57009 - not sure why it didn't close this PR, I didn't do a rebase. Thanks for the work on this one!

@tyler-ball tyler-ball closed this May 2, 2016

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