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: Fix issue updating ElasticBeanstalk Environment Settings #7777

Merged
merged 2 commits into from
Aug 7, 2016

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Jul 22, 2016

Fixes the logic that updated settings for Elastic Beanstalk Environments.
Because the update is done in the same API call, we need to split removals /
additions.

Fixes #6890

if len(add) > 0 {
for _, r := range rm {
for _, a := range add {
if *r.Namespace == *a.Namespace && *r.OptionName == *a.OptionName {
Copy link
Contributor

Choose a reason for hiding this comment

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

@catsby do we need to take into account the ResourceName setting attribute here as well? I have a feeling this may not work as intended with more than one scheduled action (http://docs.aws.amazon.com/elasticbeanstalk/latest/dg/command-options-general.html#command-options-general-autoscalingscheduledaction)

This attribute can be a bit frustrating, because it is only documented for scheduled actions, but is returned by the api for some other settings. To avoid the issues that caused I had to do this https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_elastic_beanstalk_environment.go#L717

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the statefile, we ignore ResourceName unless it's a scheduled action. I think here I'll check if the local setting has ResourceName and then check if they match, continuing if they do not match. If we don't have it set locally, I'll just continue on as we have here. Does that sound reasonable?

Copy link
Contributor Author

@catsby catsby Jul 28, 2016

Choose a reason for hiding this comment

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

I added a check in 0cec643 ea8f161 that I think covers this

Copy link
Contributor

Choose a reason for hiding this comment

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

@catsby Sorry for the delayed response here, been out for a bit. This sounds good to me!

Thanks!

@catsby catsby force-pushed the b-aws-beanstalk-env-settings-update branch 2 times, most recently from a83c1cf to a507cbc Compare July 28, 2016 20:12
Fixes the logic that updated settings for Elastic Beanstalk Environments.
Because the update is done in the same API call, we need to split removals /
additions.

Fixes #6890
@catsby catsby force-pushed the b-aws-beanstalk-env-settings-update branch from a507cbc to 0cec643 Compare July 28, 2016 20:13
@catsby
Copy link
Contributor Author

catsby commented Jul 28, 2016

updated with acc test

@catsby catsby force-pushed the b-aws-beanstalk-env-settings-update branch from 0cec643 to ea8f161 Compare July 28, 2016 20:16
@stack72 stack72 merged commit 6edf1b4 into master Aug 7, 2016
@stack72 stack72 deleted the b-aws-beanstalk-env-settings-update branch August 7, 2016 07:35
@stack72
Copy link
Contributor

stack72 commented Aug 7, 2016

LGTM!

@ghost
Copy link

ghost commented Apr 23, 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 Apr 23, 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.

Removing Elastic Beanstalk Environment Variables
3 participants