-
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: aws_db_option_group #4401
Conversation
716fd4d
to
85613a4
Compare
edcf744
to
1ddf9de
Compare
@@ -362,8 +372,8 @@ func resourceAwsDbInstanceCreate(d *schema.ResourceData, meta interface{}) error | |||
opts.TdeCredentialArn = aws.String(attr.(string)) | |||
} | |||
|
|||
if attr, ok := d.GetOk("storage_type"); ok { | |||
opts.StorageType = aws.String(attr.(string)) | |||
if attr, ok := d.GetOk("option_group_name"); ok { |
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.
was it intentional to remove storage_type
here?
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.
You are correct - this is now fixed
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.
Looks like this section now has two option_group_name
clauses (L362 & L382).
Looks like a promising start! I have some comments/asks that I've made, let me know what you think |
790a568
to
9c7d0dc
Compare
This is looking good. Anything else needed to get this into the next release? |
@fromonesrc this just needs another check to make sure everyone is happy - I only finished up the enhancements in the past week. As soon as someone from the core team is happy, then we can merge :) |
Thanks for your work on this! |
parameter_group_name = "default.mysql5.6" | ||
option_group_name = "${aws_db_option_group.bar.option_group_name}" | ||
}`, rand.New(rand.NewSource(time.Now().UnixNano())).Int()) | ||
|
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.
Is this full config necessary for the option group test? Seems like we could trim it down to just what that test needs to assert its stuff.
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.
We could probably use the new acctest.RandInt()
here for ensuring the names are unique?
I got the last problem fixed. I needed to serialize the option_settings as part of resourceAwsDbOptionHash. |
@stack72 is this something you're working on or looking for help with? |
Can we also have |
@stack72 I can update my pull request to your branch with the option_name & option_settings if that will help move things along |
Addressing @phinze's comments from hashicorp#4401
Revert "Addressing @phinze's comments from hashicorp#4401"
ec19802
to
15befd0
Compare
FYI, I am just working on getting the tests working here and then it will be ready to go |
15befd0
to
43cea49
Compare
ok this is now pending final review. I fixed up the comments @phinze gave (except the building of IAM - I have a follow up task for this once #5030 settles)
|
088c973
to
0844013
Compare
0844013
to
88a1b7e
Compare
Change the AWS DB Instance to now include the DB Option Group param. Adds a test to prove that it works Add acceptance tests for the AWS DB Option Group work. This ensures that Options can be added and updated Documentation for the AWS DB Option resource
88a1b7e
to
8dc123f
Compare
Looks good now! |
Looks like the test added to DB Instance was not updated, I patched it in #6558 |
Sorry! On Mon, May 9, 2016 at 6:33 PM, Clint notifications@github.com wrote:
|
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. |
Scaffold the AWS DB Option Group resource