-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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 support for aws autoscaling lifecycle hooks. #3351
Conversation
@xetorthio I can't see where you tell the LifeCycle Hook which ASG to attach to? I can see in your code the following:
But autoscaling_group_name is not in the schema for this resource. Wouldn't the schema have to be as follows:
|
@stack72 you are right! I forgot to add the |
nice one :) FYI i added some docs for this ( #3403 ) |
@xetorthio I have a little bit more feedback on this. Right now (since you updated the PR), the tests do not pass:
To make these pass, you need to register the new resource with the provider.go (/builtin/providers/aws/provider.go)
|
I think the reason why I didn't get this error is because I am not running acceptance tests. Am I correct? Btw, I just added the missing line |
@xetorthio excellent. The acceptance tests are not run on build. I always run them as a sanity check on whether the resource actually works These should work now. |
@stack72 @apparentlymart Merged the docs into this PR |
@xetorthio looks like you still have a merge conflict. This is because of builtin/providers/aws/provider.go |
29f2fa2
to
4f35de0
Compare
@stack72 @apparentlymart I just rebased this PR and fixed the conflict. Please let me know if anything else is missing! |
return fmt.Errorf("Error putting lifecycle hook: %s", err) | ||
} | ||
|
||
d.SetId(d.Get("name").(string)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is the only real difference between Create
and Update
here. It's harmless to call d.SetId
within Update
, if you'd like to just combine Create
and Update
into a single resourceAwsAutoscalingLifecycleHookPut
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes lots of sense! Thanks for finding this!
@xetorthio thanks for working on this! I left one minor comment inline. When I ran the acceptance tests I got this error:
Any idea what's causing that? Could it be that the role and notification target ARNs contain account ids that differ from my account? If so it might work better to create an IAM role and/or a notification target as part of the test configuration and interpolate the ARNs, so that they will point at resources that are valid for the account running the test. |
@apparentlymart I made the change you recommended and also changed the test so it creates the necessary iam role, policy and sqs queue so we don't depend on anything external. Hopefully I didn't miss anything since it is hard to test locally. |
@xetorthio Thanks for the follow-up commit. This new version seems to cause a different acceptance test failure:
I tried to fix this myself by adding the I also did some other fixups which I was just going to include in the merge, but since this test failure blocked me from merging I'll just note them here:
If we can figure out what is the right config to get the acceptance tests working, along with those minor things above, then I think this is ready to merge. Thanks again for all your work on this! |
@apparentlymart I fixed all the things you mentioned and also the acceptance test. At the end the best was to run it on my own account and make sure it works before pushing, which is what I should've done in the first place (sorry about that). If you think this PR is ready to merge, let me know before doing so, I want to squash all commits, so history is nicer and not so messy. |
@xetorthio this looks good to me now. As you squash, please retain @stack72's documentation commit as a separate one so he'll be properly attributed in the history. Thanks again for all your effort in doing this, and for your patience in responding to all of the feedback! |
658505c
to
57c80a0
Compare
@apparentlymart all my commits were squashed and of course I kept @stack72's commit |
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. |
This PR adds the ability to manage AWS Autoscaling Group Lifecycle Hooks.
It adds a new resource
aws_autoscaling_lifecycle_hook
which has a few attributes the can be set, like: