search

hashicorp / terraform-provider-aws

The AWS Provider enables Terraform to manage AWS resources.
https://registry.terraform.io/providers/hashicorp/aws
Mozilla Public License 2.0
9.83k stars 9.18k forks source link

Proposal: Combine Singular Data Source and Resource Acceptance Testing #8223

Open bflad opened 5 years ago

bflad commented 5 years ago

Community Note

Background

We have hit another regression scenario where a resource update for new functionality broke the respective data source.

Historically, the main reason for these types of issues was that the data source shared the Read function with the resource. When the data source does not have its schema (and preferably its acceptance testing) also updated, the mismatch in the data source can cause it return errors and prevent usage. We have since gone through and refactored many of the data sources to implement their own Read function to make the appropriate root level attribute d.Set() calls.

This last occurrence was more subtle though and involved a new nested schema attribute under a configuration block. Typically for these configuration block attributes, the data source and resource Read functions call out to a shared flattenXXX() function to convert an SDK object from the API response into the expected Terraform state. Since there is yet another tightly coupled implementation and most d.Set() calls for configuration blocks are now correctly performing error checking, the mismatch of schemas causes the data source to return errors and prevent usage.

Maintainers are procedurally accustomed to running acceptance testing for only the particular resource pattern (e.g. TestAccAWSServiceThing_) OR data source pattern (e.g. TestAccAWSServiceThingDataSource_) affected by code changes as running the entire acceptance test suite is unrealistic due to time and money constraints.

Currently, we write bespoke data source acceptance testing in aws/data_source_RESOURCE_test.go files. While many variations of resource test cases are created in the aws/resource_RESOURCE_test.go file, there is very little parity to when a matching data source test case is created. The data source testing typically only covers basic use cases and the test configurations are usually exact duplicates of resource test configurations plus a data source declaration.

Respective data sources are very common feature requests which are showing no signs of decrease, so problems outlined above will only continue to grow in size over time.

Regression Issue References:

Proposal

Migrate singular data source testing to the majority of resource test cases by adding a data source declaration to the each resource configuration along with relevant data source attribute checks.

This solves a few problems:

Acceptance Testing Configuration Updates

Generally data source configurations can use a straightforward argument reference to the resource identifier

# existing test configuration
resource "aws_example" "test" {
  # ... other configuration ...
}

# new configuration
data "aws_example" "test" {
  id = "${aws_example.test.id}"
}

More advanced data source cases can be handled by keeping the existing data source configurations and test checks as-is but updating the data source test names to match the resource test naming pattern.

Acceptance Testing Function Updates

Essentially we can take the current style of data source testing that prefers to utilize resource.TestCheckResourceAttrPair() to ensure the resource and data source attributes properly exist and have matching values where expected.

// ... existing checks ....
resource.TestCheckResourceAttr(resourceName, "name", rName),
// new checks:
resource.TestCheckResourceAttrPair(datasourceName, "arn", resourceName, "arn"),
// ... repeat for relevant attributes ...

Affected Resources and Data Sources

Abandoned Ideas

github-actions[bot] commented 3 years ago

Marking this issue as stale due to inactivity. This helps our maintainers find and focus on the active issues. If this issue receives no comments in the next 30 days it will automatically be closed. Maintainers can also remove the stale label.

If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thank you!

bflad commented 3 years ago

Bumping to keep this open.