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.

Sign up for GitHub

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

Jump to bottom

Add support docdb cluster parameter group resource #7090

Merged
merged 4 commits into from
Jan 10, 2019
Merged

Add support docdb cluster parameter group resource #7090

merged 4 commits into from
Jan 10, 2019

Conversation

teraken0509
Copy link
Contributor

@teraken0509 teraken0509 commented Jan 10, 2019 •
edited by bflad
Loading

Reference #7077

Changes proposed in this pull request:

  • Add docdb tag resource
  • Add resource aws_docdb_cluster_parameter_group

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSDocDBClusterParameterGroup_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSDocDBClusterParameterGroup_ -timeout 120m
=== RUN   TestAccAWSDocDBClusterParameterGroup_basic
=== PAUSE TestAccAWSDocDBClusterParameterGroup_basic
=== RUN   TestAccAWSDocDBClusterParameterGroup_namePrefix
=== PAUSE TestAccAWSDocDBClusterParameterGroup_namePrefix
=== RUN   TestAccAWSDocDBClusterParameterGroup_generatedName
=== PAUSE TestAccAWSDocDBClusterParameterGroup_generatedName
=== RUN   TestAccAWSDocDBClusterParameterGroup_Description
=== PAUSE TestAccAWSDocDBClusterParameterGroup_Description
=== RUN   TestAccAWSDocDBClusterParameterGroup_disappears
=== PAUSE TestAccAWSDocDBClusterParameterGroup_disappears
=== RUN   TestAccAWSDocDBClusterParameterGroup_Parameter
=== PAUSE TestAccAWSDocDBClusterParameterGroup_Parameter
=== RUN   TestAccAWSDocDBClusterParameterGroup_Tags
=== PAUSE TestAccAWSDocDBClusterParameterGroup_Tags
=== CONT  TestAccAWSDocDBClusterParameterGroup_basic
=== CONT  TestAccAWSDocDBClusterParameterGroup_disappears
=== CONT  TestAccAWSDocDBClusterParameterGroup_Tags
=== CONT  TestAccAWSDocDBClusterParameterGroup_Parameter
=== CONT  TestAccAWSDocDBClusterParameterGroup_generatedName
=== CONT  TestAccAWSDocDBClusterParameterGroup_Description
=== CONT  TestAccAWSDocDBClusterParameterGroup_namePrefix
--- PASS: TestAccAWSDocDBClusterParameterGroup_disappears (23.16s)
--- PASS: TestAccAWSDocDBClusterParameterGroup_generatedName (33.17s)
--- PASS: TestAccAWSDocDBClusterParameterGroup_namePrefix (33.18s)
--- PASS: TestAccAWSDocDBClusterParameterGroup_Description (33.30s)
--- PASS: TestAccAWSDocDBClusterParameterGroup_basic (33.41s)
--- PASS: TestAccAWSDocDBClusterParameterGroup_Parameter (51.23s)
--- PASS: TestAccAWSDocDBClusterParameterGroup_Tags (69.49s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	69.561s
...

@ghost ghost added size/XXL Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. provider Pertains to the provider itself, rather than any interaction with AWS. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jan 10, 2019
@teraken0509 teraken0509 force-pushed the feature/add-support-docdb-cluster-parameter-group-resource branch from 1636444 to c1878a4 Compare January 10, 2019 10:11
@ewbankkit ewbankkit mentioned this pull request Jan 10, 2019
Closed
@okonon
Copy link

okonon commented Jan 10, 2019 •
edited
Loading

@ewbankkit @kterada0509 this is awesome and thanks for your quick turnaround!!!! long awaited feature for us

@okonon
Copy link

okonon commented Jan 10, 2019

@kterada0509 @ewbankkit does this support encryption at rest --storage-encrypted ?

bflad
bflad reviewed Jan 10, 2019
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Hey @kterada0509! Thanks for submitting this. A few minor things and should be good to go. 👍

aws/resource_aws_docdb_cluster_parameter_group.go Show resolved Hide resolved
aws/resource_aws_docdb_cluster_parameter_group.go Outdated
}

d.SetId(aws.StringValue(createOpts.DBClusterParameterGroupName))
log.Printf("[INFO] Neptune Cluster Parameter Group ID: %s", d.Id())
Copy link
Contributor

Choose a reason for hiding this comment

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

Copypasta 🍝 and we really don't need this log line 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed logging

aws/resource_aws_docdb_cluster_parameter_group.go
d.SetId(aws.StringValue(createOpts.DBClusterParameterGroupName))
log.Printf("[INFO] Neptune Cluster Parameter Group ID: %s", d.Id())

d.Set("arn", resp.DBClusterParameterGroup.DBClusterParameterGroupArn)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maintainer note: for consistency reasons with RDS/Neptune, setting arn in the Create here and calling Update after Create is okay for now -- they will all get similar fixes in the future.

aws/resource_aws_docdb_cluster_parameter_group.go Outdated

describeParametersResp, err := conn.DescribeDBClusterParameters(describeParametersOpts)
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: It'd be nice to return error context here for operators and code maintainers

Suggested change
return err
return fmt.Errorf("error reading DocDB Cluster Parameter Group (%s) parameters: %s", d.Id(), err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

aws/resource_aws_docdb_cluster_parameter_group.go Outdated
})

if err != nil {
log.Printf("[DEBUG] Error retrieving tags for ARN: %s", arn)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should return an error here instead of a debug log:

Suggested change
log.Printf("[DEBUG] Error retrieving tags for ARN: %s", arn)
return fmt.Errorf("error listing tags for DocDB Cluster Parameter Group (%s): %s", d.Id(), err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

website/docs/r/docdb_cluster_parameter_group.html.markdown Outdated
description = "docdb cluster parameter group"

parameter {
name = "neptune_enable_audit_log"
Copy link
Contributor

Choose a reason for hiding this comment

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

Copypasta 🍝 Lets pick a DocDB one 😄 https://docs.aws.amazon.com/documentdb/latest/developerguide/db-cluster-parameters.html

Suggested change
name = "neptune_enable_audit_log"
name = "tls"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

website/docs/r/docdb_cluster_parameter_group.html.markdown Outdated

parameter {
name = "neptune_enable_audit_log"
value = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

And a matching value from our testing 😉

Suggested change
value = 1
value = "enabled"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

website/aws.erb Outdated
<ul class="nav nav-visible">

<li<%= sidebar_current("docs-aws-resource-docdb-cluster-parameter-group") %>>
<a href="/docs/providers/aws/r/docdb_cluster_parameter_group.html">aws_dax_cluster</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Copypasta 🍝

Suggested change
<a href="/docs/providers/aws/r/docdb_cluster_parameter_group.html">aws_dax_cluster</a>
<a href="/docs/providers/aws/r/docdb_cluster_parameter_group.html">aws_docdb_cluster_parameter_group</a>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@bflad bflad added waiting-response Maintainers are waiting on response from community or contributor. new-resource Introduces a new resource. labels Jan 10, 2019
@bflad
Copy link
Contributor

bflad commented Jan 10, 2019

I'd also change your PR comment to Reference: #7077 since this shouldn't close the issue. 😉

@teraken0509 teraken0509 force-pushed the feature/add-support-docdb-cluster-parameter-group-resource branch from c1878a4 to a31ec1b Compare January 10, 2019 16:55
@ewbankkit
Copy link
Contributor

@okonon All kudos to @kterada0509 for this, I just opened the issue 😄.
It looks like DocumentDB encryption-at-rest is configured when creating the cluster so this will be an attribute of the aws_docdb_cluster resource.

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 10, 2019
@teraken0509 teraken0509 force-pushed the feature/add-support-docdb-cluster-parameter-group-resource branch from a31ec1b to 940943b Compare January 10, 2019 17:00
@teraken0509
Copy link
Contributor Author

@bflad
Thanks for review.
I fixed your review.

@bflad bflad added this to the v1.55.0 milestone Jan 10, 2019
bflad
bflad approved these changes Jan 10, 2019
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Great work, @kterada0509, thanks! 🚀

--- PASS: TestAccAWSDocDBClusterParameterGroup_disappears (3.16s)
--- PASS: TestAccAWSDocDBClusterParameterGroup_generatedName (4.73s)
--- PASS: TestAccAWSDocDBClusterParameterGroup_Description (4.74s)
--- PASS: TestAccAWSDocDBClusterParameterGroup_basic (4.77s)
--- PASS: TestAccAWSDocDBClusterParameterGroup_namePrefix (4.78s)
--- PASS: TestAccAWSDocDBClusterParameterGroup_Parameter (12.20s)
--- PASS: TestAccAWSDocDBClusterParameterGroup_Tags (12.53s)

@bflad bflad merged commit 8d937ec into hashicorp:master Jan 10, 2019
bflad added a commit that referenced this pull request Jan 10, 2019
@bflad
Update CHANGELOG for #7090
c757e3c
@teraken0509 teraken0509 mentioned this pull request Jan 10, 2019
Merged
@bflad
Copy link
Contributor

bflad commented Jan 10, 2019

This has been released in version 1.55.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@teraken0509 teraken0509 deleted the feature/add-support-docdb-cluster-parameter-group-resource branch March 5, 2020 14:03
@ghost
Copy link

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

@ghost ghost locked and limited conversation to collaborators Mar 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
documentation Introduces or discusses updates to documentation. new-resource Introduces a new resource. provider Pertains to the provider itself, rather than any interaction with AWS. size/XXL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v1.55.0
Development

Successfully merging this pull request may close these issues.
4 participants
@teraken0509 @okonon @bflad @ewbankkit