create_before_destroy for parameter groups, explicit dependencies

[syphernl]

what

• Make parameter groups create_before_destroy
• Make the name for parameter & option groups unique to ensure we can create new one's
• Add explicit dependencies

why

• You cannot delete a parameter/option group while it is in use, but you can update a database with a new parameter/option group, so in order to change parameter/option groups, you need to create a new one, install it, then delete the old one.
• Some update and destroy operations were failing because resources were being deleted in the wrong order.

references

• The same changes have been done in https://github.com/cloudposse/terraform-aws-rds-cluster/pull/110 earlier

[syphernl]

Our use-case:

• We wanted to upgrade our RDS instance from Postgresql 12 to 13
• RDS instance needed new Parameter and Option groups with a new family (postgres13 instead of postgres12)
• Terraform could not remove the existing groups, because they were in use

[syphernl]

Your changes can be a big surprise for that one who decided to update version of module. Could you keep the current behaviour and introduce the new variable use_name_prefix for your case. As example see https://github.com/cloudposse/terraform-aws-security-group/blob/master/main.tf

I'm not sure whether that's the right approach for this, since this is required if you want to upgrade major versions of RDS for instance. It doesn't break anything except for changing the naming of the parameter and option groups.

This PR is following the same approach as done in https://github.com/cloudposse/terraform-aws-rds-cluster/pull/110. If the use_name_prefix is the way to go it should probably also be modified accordingly.

[SweetOps]

I'm not sure whether that's the right approach for this, since this is required if you want to upgrade major versions of RDS for instance. It doesn't break anything except for changing the naming of the parameter and option groups.

I pretty sure that will trigger resources(aws_db_parameter_group, aws_db_option_group) to re-create. And I talking about the case when module in use and I wanna just bump version of module to next one.

[syphernl]

I pretty sure that will trigger resources(aws_db_parameter_group, aws_db_option_group) to re-create.

In case of upgrades (e.g. postgres from 12 to 13) without these changes the state cannot be applied, because the group name is already in use and cannot be upgraded as-is.

And I talking about the case when module in use and I wanna just bump version of module to next one.

Module upgrades shouldn't result in new groups (once they are initially created with a prefix) unless group parameters are being modified.

[SweetOps]

once they are initially created with a prefix

exactly! A lot of end-customers uses this module in production and for them your changes are breaking. As I wrote before you need just add one var to save current behaviour and satisfy your case.

[Nuru]

/test all

@SweetOps changes to the names of parameter groups and options groups will cause them to be recreated, but it is not a problem to change them in existing databases, so the change should not actually be breaking anything.

[SweetOps]

changes to the names of parameter groups and options groups will cause them to be recreated, but it is not a problem to change them in existing databases, so the change should not actually be breaking anything.

@Nuru these changes requires RDS instance reboot or it'll reboot it immediately (or stuck in pending-reboot)... Just for me potential down-time due not necessary renaming is breaking change

[Nuru]

changes to the names of parameter groups and options groups will cause them to be recreated, but it is not a problem to change them in existing databases, so the change should not actually be breaking anything.

@Nuru these changes requires RDS instance reboot or it'll reboot it immediately (or stuck in pending-reboot)... Just for me potential down-time due not necessary renaming is breaking change

@SweetOps Fair enough, we can call this a breaking change if you want. I am OK with that, but we can ask @osterman and @aknysh for their opinions. The reason I'm OK with a breaking change here is that otherwise the module simply cannot update or modify the parameter group or option group, so I consider the module broken.

In any case, someone needs to fix the test to expect the generated name:

--- FAIL: TestExamplesComplete (684.91s)
    examples_complete_test.go:51: 
            Error Trace:    examples_complete_test.go:51
            Error:          Not equal: 
                            expected: "eg-test-rds-test"
                            actual  : "eg-test-rds-test-20210304211515834400000002"

                            Diff:
                            --- Expected
                            +++ Actual
                            @@ -1 +1 @@
                            -eg-test-rds-test
                            +eg-test-rds-test-20210304211515834400000002
            Test:           TestExamplesComplete
    examples_complete_test.go:56: 
            Error Trace:    examples_complete_test.go:56
            Error:          Not equal: 
                            expected: "eg-test-rds-test"
                            actual  : "eg-test-rds-test-20210304211515833100000001"

                            Diff:
                            --- Expected
                            +++ Actual
                            @@ -1 +1 @@
                            -eg-test-rds-test
                            +eg-test-rds-test-20210304211515833100000001
            Test:           TestExamplesComplete

[aknysh]

@syphernl Sorry I hijacked your PR here https://github.com/cloudposse/terraform-aws-rds/pull/110. Needed to fix the tests, but all the functionality you wanted to implement is already in the master branch. Thank you.