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

add resource aws_rds_cluster_parameter_group #5269

Merged
merged 4 commits into from
May 18, 2016
Merged

add resource aws_rds_cluster_parameter_group #5269

merged 4 commits into from
May 18, 2016

Conversation

Flygsand
Copy link

This PR adds support for RDS Cluster Parameter Groups (see #5031, #5023). It's more or less a carbon copy of aws_db_parameter_group.

I have a small question regarding error handling. aws_db_parameter_group compares against DBParameterGroupNotFoundFault, not DBParameterGroupNotFound, which is the error code I've been observing in docs and during tests. Is that correct? I'm using DBParameterGroupNotFound for now.

@chrisfu
Copy link

chrisfu commented Mar 17, 2016

+1.

@phinze
Copy link
Contributor

phinze commented Mar 18, 2016

I have a small question regarding error handling. aws_db_parameter_group compares against DBParameterGroupNotFoundFault, not DBParameterGroupNotFound, which is the error code I've been observing in docs and during tests. Is that correct? I'm using DBParameterGroupNotFound for now.

It's possible that the NotFoundFault was an older version of this error message, so you should definitely be catching the version of the error message you're seeing in current API behavior. 👍

d.SetPartial("name")
d.SetPartial("family")
d.SetPartial("description")
d.Partial(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

The Partial mode handling is only relevant when multiple API calls are required to accomplish an overall change. So in this case these lines are a noop and can be removed. 👍

@phinze
Copy link
Contributor

phinze commented Mar 18, 2016

Wow @protomouse this is looking really, really good!

A few inline comments for you to check out, but really great work here so far! 😀

@Flygsand
Copy link
Author

@phinze Sorry for the hold up. Thanks for reviewing this. I've fixed these things in separate commits. If it looks good, I'll rebase.

@stack72
Copy link
Contributor

stack72 commented May 12, 2016

Hi @protomouse

I am trying to do a round up of some PRs and came across this one. Ran the tests (after noticing you made the changes that @phinze suggested) but got this:

make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSDBClusterParameterGroup' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSDBClusterParameterGroup -timeout 120m
=== RUN   TestAccAWSDBClusterParameterGroup_basic
--- PASS: TestAccAWSDBClusterParameterGroup_basic (50.46s)
=== RUN   TestAccAWSDBClusterParameterGroup_disappears
--- FAIL: TestAccAWSDBClusterParameterGroup_disappears (17.46s)
    testing.go:234: Step 0 error: Error on follow-up refresh: 1 error(s) occurred:

        * aws_rds_cluster_parameter_group.bar: DBParameterGroupNotFound: DBClusterParameterGroup cluster-parameter-group-test-terraform not found.
            status code: 404, request id: 04358700-1891-11e6-ba4d-d1bc4486f6ca
=== RUN   TestAccAWSDBClusterParameterGroupOnly
--- PASS: TestAccAWSDBClusterParameterGroupOnly (28.95s)
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/aws    96.900s

This test failure is because of this line:

https://github.com/hashicorp/terraform/pull/5269/files#diff-3cbb9380152a2a5c4b43e2b65dfbfc11R113

This should be something like:

if err != nil {
        if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "DBParameterGroupNotFound" {
            log.Printf("[WARN] DB Cluster Parameter Group (%s) not found, error code (404)", d.Id())
            d.SetId("")
            return nil
        }

        return err
    }

If you can fix this up, we are home free for the merge :)

Paul

@stack72 stack72 added the waiting-response An issue/pull request is waiting for a response from the community label May 12, 2016
@stack72 stack72 self-assigned this May 12, 2016
@stack72 stack72 merged commit 0d82b4d into hashicorp:master May 18, 2016
@stack72
Copy link
Contributor

stack72 commented May 18, 2016

Manually merged to get the DBParameterGroupNotFound included

@ghost
Copy link

ghost commented Apr 25, 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 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement provider/aws waiting-response An issue/pull request is waiting for a response from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants