-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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: Support "custom JSON" for OpsWorks layers #4272
provider/aws: Support "custom JSON" for OpsWorks layers #4272
Conversation
👍 |
1 similar comment
👍 |
Updated this PR to complete the following:
@radeksimko Anything else I need to do to get this merged? |
+1 this would be super helpful for us. 😄 |
@@ -377,6 +392,7 @@ func (lt *opsworksLayerType) Update(d *schema.ResourceData, client *opsworks.Ops | |||
CustomInstanceProfileArn: aws.String(d.Get("custom_instance_profile_arn").(string)), | |||
CustomRecipes: lt.CustomRecipes(d), | |||
CustomSecurityGroupIds: expandStringSet(d.Get("custom_security_group_ids").(*schema.Set)), | |||
CustomJson: aws.String(d.Get("custom_json").(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.
What happens here if CustomJson is not defined? I think it will have to change to being outside the struct declaration and then check if it is defined
Thoughts?
Hi @mrjefftang Thanks so much for the PR here. I have left a couple of comments about empty params 1 other point is that the both tests have been updated to have custom_json. This means we have no longer got a test inplace to make sure that custom_json is truly optional Please can you modify this [test[(https://github.com/mrjefftang/terraform/blob/opsworks_custom_json/builtin/providers/aws/resource_aws_opsworks_custom_layer_test.go#L18) to remove the custom_json param and rerun your tests? It may be used to make an additional test cover the functionality of custom_json. This will ensure that only the basic config necessary is covered in our most basic test. This is a pattern that a lot of the resources follow Thanks Paul |
Hi @stack72 , I've implemented your suggestions. |
Thanks @mrjefftang Currently running the tests to make sure all looks ok Paul |
Hi @mrjefftang So on running the tests, I get state changes
This is saying that a plan -> apply - > plan will always show that changes are needed to be made Paul |
@stack72 I am unable to reproduce that failure. That output indicates that it's trying to update |
@mrjefftang sorry for the really long turnaround time on this one, and thanks for your time and patience! I've just merged your change, with the addition of a minor follow-up change to set the |
@stack72 I was also unable to reproduce that test failure you saw there, and manual testing indicated this worked, so I proceeded with the merge. Can you still repro that test failure? I'm wondering if there's something different about your AWS account compared to mine... in earlier opsworks changes it's seemed that the Opsworks console sometimes "secretly" creates or updates objects in ways that the raw API does not, so someone who has used the UI before gets different results than someone who hasn't. If you're still seeing the error maybe we can make a new issue for that, since I agree with @mrjefftang that it doesn't seem related to this change at first glance. |
I can reproduce this on every test run Paul |
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. |
No description provided.