r/aws_db_parameter_group, r/aws_rds_cluster_parameter_group: Support resetting parameters #5728

stefansundin · 2018-08-30T02:42:40Z

If you create a parameter group like so:

resource "aws_rds_cluster_parameter_group" "cluster" {
  name        = "test-cluster"
  family      = "aurora-mysql5.7"

  parameter {
    name         = "binlog_format"
    value        = "MIXED"
    apply_method = "pending-reboot"
  }
}

And then uncomment the parameter block and apply again, it doesn't actually do anything. This PR fixes that.

It is sending the parameter value in the reset request, but this doesn't seem to be causing any problems. I inspected a request in the RDS web console, and it also sends the value (and a lot of other stuff).

I tried setting 22 parameters and then resetting them all in one request, and it worked. I was able to re-produce the error of setting more than 20 parameters at the same time.. Is there anywhere else that this limit is documented?

Let me know what you think!! :)

(Relates: #661)

…uster_parameter_group.

stefansundin · 2018-08-30T02:48:14Z

Oh, and I will write tests for this tomorrow!

stefansundin · 2018-08-31T18:42:54Z

I have been trying to add a test that would detect when the code fails to reset the parameter, but I am having issues. So this is with my new code commented out.

I added a new commit with my test code. With that test, and the new code commented out, I expect the test to fail, but it does not.

So my understanding is that in the test steps in TestAccAWSDBParameterGroup_basic, they all use the same parameter group, and it isn't destroyed until after the last test step. So the purpose of my new test step is to try to undo the new parameters that were added in the step before, and then check that collation_connection and collation_server are no longer user defined.

Any help on this would be awesome! Thank you!

ryndaniels

Hi @stefansundin - thanks so much for opening this PR! There's a few minor changes we'd like to see to this, so if you're able to make those changes, we can keep moving forward with getting this merged. Let me know if you have any questions on anything 🙂

ryndaniels · 2019-10-04T13:27:47Z

aws/resource_aws_db_parameter_group.go

@@ -295,6 +295,26 @@ func resourceAwsDbParameterGroupUpdate(d *schema.ResourceData, meta interface{})
 			}
 			d.SetPartial("parameter")
 		}
+
+		// Reset parameters that have been removed
+		parameters, err = expandParameters(os.Difference(ns).List())


I'm seeing in the ResetDBParameterGroupInput SDK documentation here that there is a documented 20 item max, so it would probably be best to add that logic in here. There's an example of doing this with a for-loop here. 🙂

ryndaniels · 2019-10-04T13:29:25Z

aws/resource_aws_db_parameter_group.go

+			parameterGroupName := d.Get("name").(string)
+			resetOpts := rds.ResetDBParameterGroupInput{
+				DBParameterGroupName: aws.String(parameterGroupName),
+				Parameters:           parameters,


It looks like by default, all parameters are reset so we should add this in to prevent that:

Suggested change

Parameters: parameters,

Parameters: parameters,

ResetAllParameters: aws.Bool(false),

ryndaniels · 2019-10-04T13:38:14Z

aws/resource_aws_db_parameter_group_test.go

+					testAccCheckAWSDBParameterGroupExists("aws_db_parameter_group.bar", &v),
+					testAccCheckAWSDBParameterGroupAttributes(&v, groupName),
+					testAccCheckAWSDBParameterNotUserDefined("aws_db_parameter_group.bar", "collation_connection"),
+					testAccCheckAWSDBParameterNotUserDefined("aws_db_parameter_group.bar", "collation_server"),


To add some additional checking to this, you could try adding something like:

Suggested change

testAccCheckAWSDBParameterNotUserDefined("aws_db_parameter_group.bar", "collation_server"),

testAccCheckAWSDBParameterNotUserDefined("aws_db_parameter_group.bar", "collation_server"),

testAccCheckNoResourceAttr("aws_db_parameter_group.bar", "parameter.2475805061.value"),

for additional testing to make sure the parameters were reset.

ryndaniels · 2019-10-04T13:40:04Z

aws/resource_aws_db_parameter_group_test.go

+			DBParameterGroupName: aws.String(rs.Primary.ID),
+			Source:               aws.String("user"),
+		}
+		fmt.Printf("test: %s\n", rs.Primary.ID)


In general, we'd prefer not to have fmt.Printfs in the code. For potential debug information, a log.Printf could work, but if these were just testing messages, could you go ahead and remove them?

ryndaniels · 2019-10-04T13:41:01Z

aws/resource_aws_rds_cluster_parameter_group.go

@@ -234,6 +234,26 @@ func resourceAwsRDSClusterParameterGroupUpdate(d *schema.ResourceData, meta inte
 			}
 			d.SetPartial("parameter")
 		}
+
+		// Reset parameters that have been removed


Could you go ahead and add a test for this change as well? Thanks!

nathanielks · 2020-01-07T00:15:25Z

@stefansundin Hey there! I was curious if you're still interested in patching this? If not, I might try picking up the torch and finishing it.

stefansundin · 2020-01-07T01:31:33Z

@nathanielks Feel free to continue this work.

ryndaniels · 2020-01-08T14:02:05Z

@nathanielks thanks for volunteering to pick this up! I've actually just started working on this myself and have got it just about ready to go, so if there's other stuff that's higher priority for you to work on, that's totally fine and we should be getting this going on our end shortly! 🙂

nathanielks · 2020-01-08T23:21:53Z

Oh brilliant, thanks @ryndaniels! Carry on, then 😄

ryndaniels · 2020-01-09T18:26:43Z

(Pulled these commits into another branch (#11540) just to simplify things on our end - thanks for getting this started @stefansundin - I'm going to close this PR to consolidate conversation on the new one! 🙂)

ghost · 2020-03-27T17:56:33Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

Support resetting parameters in aws_db_parameter_group and aws_rds_cl…

166ca35

…uster_parameter_group.

bflad added bug Addresses a defect in current functionality. service/rds Issues and PRs that pertain to the rds service. labels Aug 30, 2018

Try to add a test.

a4963dd

stefansundin mentioned this pull request Oct 13, 2018

aws_dynamodb_table: Work around tagging limitation in dynamodb-local #6149

Merged

aeschright requested a review from a team June 25, 2019 21:27

ryndaniels mentioned this pull request Oct 4, 2019

Reset parameter group values when they are removed from config #10161

Closed

ryndaniels suggested changes Oct 4, 2019

View reviewed changes

ryndaniels added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 4, 2019

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 7, 2020

ryndaniels self-assigned this Jan 8, 2020

ryndaniels mentioned this pull request Jan 9, 2020

Support resetting of parameter group values #11540

Merged

ryndaniels closed this Jan 9, 2020

ghost locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/aws_db_parameter_group, r/aws_rds_cluster_parameter_group: Support resetting parameters #5728

r/aws_db_parameter_group, r/aws_rds_cluster_parameter_group: Support resetting parameters #5728

stefansundin commented Aug 30, 2018 •

edited by ryndaniels

Loading

stefansundin commented Aug 30, 2018

stefansundin commented Aug 31, 2018

ryndaniels left a comment

ryndaniels Oct 4, 2019

ryndaniels Oct 4, 2019

ryndaniels Oct 4, 2019

ryndaniels Oct 4, 2019

ryndaniels Oct 4, 2019

nathanielks commented Jan 7, 2020

stefansundin commented Jan 7, 2020

ryndaniels commented Jan 8, 2020

nathanielks commented Jan 8, 2020

ryndaniels commented Jan 9, 2020

ghost commented Mar 27, 2020

	Parameters: parameters,
	Parameters: parameters,
	ResetAllParameters: aws.Bool(false),

	testAccCheckAWSDBParameterNotUserDefined("aws_db_parameter_group.bar", "collation_server"),
	testAccCheckAWSDBParameterNotUserDefined("aws_db_parameter_group.bar", "collation_server"),
	testAccCheckNoResourceAttr("aws_db_parameter_group.bar", "parameter.2475805061.value"),

r/aws_db_parameter_group, r/aws_rds_cluster_parameter_group: Support resetting parameters #5728

r/aws_db_parameter_group, r/aws_rds_cluster_parameter_group: Support resetting parameters #5728

Conversation

stefansundin commented Aug 30, 2018 • edited by ryndaniels Loading

stefansundin commented Aug 30, 2018

stefansundin commented Aug 31, 2018

ryndaniels left a comment

Choose a reason for hiding this comment

ryndaniels Oct 4, 2019

Choose a reason for hiding this comment

ryndaniels Oct 4, 2019

Choose a reason for hiding this comment

ryndaniels Oct 4, 2019

Choose a reason for hiding this comment

ryndaniels Oct 4, 2019

Choose a reason for hiding this comment

ryndaniels Oct 4, 2019

Choose a reason for hiding this comment

nathanielks commented Jan 7, 2020

stefansundin commented Jan 7, 2020

ryndaniels commented Jan 8, 2020

nathanielks commented Jan 8, 2020

ryndaniels commented Jan 9, 2020

ghost commented Mar 27, 2020

stefansundin commented Aug 30, 2018 •

edited by ryndaniels

Loading