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

New Resource aws_rds_cluster_activity_stream #22097

Merged
merged 51 commits into from
Apr 6, 2022
Merged

New Resource aws_rds_cluster_activity_stream #22097

merged 51 commits into from
Apr 6, 2022

Conversation

jdstuart
Copy link
Contributor

@jdstuart jdstuart commented Dec 7, 2021

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #14243
Relates #18980


Description: This PR adds support for the Amazon Aurora Database Activity Streams.

A lot of credit to @Fedomn that did a bunch of the work in #14243.


Output from acceptance testing:

$ make testacc TESTS=TestAccAWSRDSClusterActivityStream PKG=rds
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/rds/... -v -count 1 -parallel 20 -run='TestAccAWSRDSClusterActivityStream' -timeout 180m
=== RUN   TestAccAWSRDSClusterActivityStream_basic
=== PAUSE TestAccAWSRDSClusterActivityStream_basic
=== RUN   TestAccAWSRDSClusterActivityStream_disappears
=== PAUSE TestAccAWSRDSClusterActivityStream_disappears
=== RUN   TestAccAWSRDSClusterActivityStream_kmsKeyId
=== PAUSE TestAccAWSRDSClusterActivityStream_kmsKeyId
=== RUN   TestAccAWSRDSClusterActivityStream_mode
=== PAUSE TestAccAWSRDSClusterActivityStream_mode
=== RUN   TestAccAWSRDSClusterActivityStream_resourceArn
=== PAUSE TestAccAWSRDSClusterActivityStream_resourceArn
=== CONT  TestAccAWSRDSClusterActivityStream_basic
=== CONT  TestAccAWSRDSClusterActivityStream_mode
=== CONT  TestAccAWSRDSClusterActivityStream_resourceArn
=== CONT  TestAccAWSRDSClusterActivityStream_disappears
=== CONT  TestAccAWSRDSClusterActivityStream_kmsKeyId
--- PASS: TestAccAWSRDSClusterActivityStream_disappears (1677.10s)
--- PASS: TestAccAWSRDSClusterActivityStream_basic (2132.00s)
--- PASS: TestAccAWSRDSClusterActivityStream_kmsKeyId (2573.85s)
--- PASS: TestAccAWSRDSClusterActivityStream_mode (2828.39s)
--- PASS: TestAccAWSRDSClusterActivityStream_resourceArn (3295.58s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/rds	3297.531s

Fedomn and others added 29 commits May 13, 2021 16:00
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Dec 7, 2021
@jdstuart
Copy link
Contributor Author

I'm having a difficult time debugging the Acceptance test code and I'm not sure why my tests are failing if it's bad code in my test or in the resource I added (or both :( ).

I use Visual Studio Code for development and successfully followed the instructions on Debugging Providers to attach a debugger to the AWS Provider. I manage to run the acceptance tests with make testacc TESTS=... from the CLI, but that doesn't enable me to debug the tests themselves. I can successfully step through the provider code, but I believe the problem is in my tests and not the provider.

Any help or ideas how I can debug the acceptance tests? @DrFaust92 (or anyone)

internal/service/rds/cluster_activity_stream_test.go Outdated Show resolved Hide resolved
}
}

func testAccCheckAWSRDSClusterActivityStreamAttributes(v *rds.DBCluster) resource.TestCheckFunc {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is these need if attributes are already checked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It checks properties returned from AWS and not only attributes stored with the resource. i.e. the ActivityStreamStatus which is expected to be in state Started

return diag.FromErr(fmt.Errorf("error retrieving RDS cluster: empty response for: %s", d.Id()))
}

if aws.StringValue(resp.ActivityStreamStatus) == rds.ActivityStreamStatusStopped {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should fold this into the find function (also will remove the dupe in acctest destory function)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you suggest the FindDBClusterByClusterArn function or do you suggest I create a new find function for the Activity Stream? The Activity Stream isn't really something you can "find" and return. It's a status property on the DB cluster.

kms_key_id = aws_kms_key.test.key_id
mode = "async"

depends_on = [aws_rds_cluster_instance.test]
Copy link
Collaborator

Choose a reason for hiding this comment

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

should avoid this and add some retry logic in create if it needs to wait for the instance to be created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there's no depends_on it can also cause a race condition when deleting the entire stack. i.e. If AWS is busy destroying either aws_rds_cluster or aws_rds_cluster_activity_stream then the destroy operation of the other will fail stating the cluster is in invalid state to perform the operation.

We can wait at initialization, but to wait at destroy would require changes to the aws_rds_cluster resources too.

@DrFaust92
Copy link
Collaborator

@jdstuart can you share errors? i dont run tests on debug so can help you with this unfortunately.

@jdstuart
Copy link
Contributor Author

When trying to debug the test in VS Code I get the following Debug Output:

Starting: /Users/jstuart/go/bin/dlv dap --check-go-version=false --listen=127.0.0.1:59554 --log-dest=3 from /Users/jstuart/dev_experimental/terraform-provider-aws/terraform-provider-aws/internal/service/rds
DAP server listening at: 127.0.0.1:59554
PASS
Process 35839 has exited with status 0
Detaching
dlv dap (35208) exited with code: 0

The debugger is attached and hit one of the breakpoints but when I continue it will just exit the process and detach.

@github-actions github-actions bot added the sweeper Pertains to changes to or issues with the sweeper. label Feb 15, 2022
@jdstuart
Copy link
Contributor Author

@DrFaust92 code is ready again for review.

Results from Acceptance Tests

make testacc TESTS=TestAccAWSRDSClusterActivityStream PKG=rds
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/rds/... -v -count 1 -parallel 20 -run='TestAccAWSRDSClusterActivityStream'  -timeout 180m
=== RUN   TestAccAWSRDSClusterActivityStream_basic
=== PAUSE TestAccAWSRDSClusterActivityStream_basic
=== RUN   TestAccAWSRDSClusterActivityStream_disappears
=== PAUSE TestAccAWSRDSClusterActivityStream_disappears
=== CONT  TestAccAWSRDSClusterActivityStream_basic
=== CONT  TestAccAWSRDSClusterActivityStream_disappears
--- PASS: TestAccAWSRDSClusterActivityStream_basic (1522.06s)
--- PASS: TestAccAWSRDSClusterActivityStream_disappears (1605.99s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/rds	1608.051s

@johnsonaj johnsonaj self-requested a review April 5, 2022 18:32
@johnsonaj
Copy link
Contributor

LGTM 🚀

% make testacc TESTS=TestAccAWSRDSClusterActivityStream_ PKG=rds
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/rds/... -v -count 1 -parallel 20 -run='TestAccAWSRDSClusterActivityStream_'  -timeout 180m
=== RUN   TestAccAWSRDSClusterActivityStream_basic
=== PAUSE TestAccAWSRDSClusterActivityStream_basic
=== RUN   TestAccAWSRDSClusterActivityStream_disappears
=== PAUSE TestAccAWSRDSClusterActivityStream_disappears
=== CONT  TestAccAWSRDSClusterActivityStream_basic
=== CONT  TestAccAWSRDSClusterActivityStream_disappears
--- PASS: TestAccAWSRDSClusterActivityStream_basic (1515.37s)
--- PASS: TestAccAWSRDSClusterActivityStream_disappears (1613.83s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/rds	1615.716s

@johnsonaj
Copy link
Contributor

@jdstuart Thanks for the contribution 🎉 .

@johnsonaj johnsonaj merged commit 44e1ff4 into hashicorp:main Apr 6, 2022
@github-actions github-actions bot added this to the v4.9.0 milestone Apr 6, 2022
@github-actions
Copy link

github-actions bot commented Apr 7, 2022

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

github-actions bot commented May 8, 2022

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. service/rds Issues and PRs that pertain to the rds service. size/XL Managed by automation to categorize the size of a PR. sweeper Pertains to changes to or issues with the sweeper. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants