Merge pull request #40835 from hashicorp/d-id-attribute-standards

docs: add id attribute standards reference

docs/id-attributes.md

@@ -0,0 +1,112 @@
+# `id` Attributes
+
+## Background
+
+Historically all resources in the Terraform AWS provider have included a read-only `id` attribute, as [Terraform Plugin SDK V2](https://developer.hashicorp.com/terraform/plugin/sdkv2) and its associated acceptance testing library require it.
+In most cases, this attribute corresponds to a unique identifier generated by AWS during resource creation.
+However, for some resources the identifier is a value provided by the user and the resulting `id` attribute inherently duplicates the value of some other required argument.
+
+With the general availability of [Terraform Plugin Framework](https://developer.hashicorp.com/terraform/plugin/framework), the separation of the provider testing functionality into its own standalone library ([`terraform-plugin-testing`](https://github.com/hashicorp/terraform-plugin-testing)), and some [corresponding enhancements](https://github.com/hashicorp/terraform-plugin-testing/pull/164) made to this library, resources are no longer **required** to have an `id` attribute.
+
+## Standard Usage
+
+As net-new resources [must be implemented](terraform-plugin-development-packages.md) with Terraform Plugin Framework, the following standard should be applied.
+
+!!! info "`id` Attribute Standard"
+    For all net-new resources, the `id` attribute should be omitted if it is redundant with an existing argument or is a combination of multiple arguments.
+    When the unique identifier is a combination of arguments, these should be delimited with a comma (`,`), and rely on the internal [`ExpandResourceID`](https://github.com/hashicorp/terraform-provider-aws/blob/v5.51.1/internal/flex/flex.go#L326-L354) function to handle splitting values within the `ImportState` method.
+    In all other cases the `id` attribute should continue to be used as it has been historically.
+
+## Examples
+
+For resources omitting an `id` argument, minor changes are required to customize the import method and acceptance test the import operation.
+For testing, the import verification [`TestStep`](https://pkg.go.dev/github.com/hashicorp/[email protected]/helper/resource#TestStep) will now require the `ImportStateVerifyIdentifierAttribute` and one of `ImportStateID` or `ImportStateIdFunc` be configured.
+Examples for a single, non-`id` identifier and a multi-part identifier are included below.
+
+### Non-`id` Identifier
+
+In this example, the `vpc_endpoint_id` argument is used as the resource identifier.
+
+`ImportState` method:
+
+```go
+func (r *resourceEndpointPrivateDNS) ImportState(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) {
+	resource.ImportStatePassthroughID(ctx, path.Root("vpc_endpoint_id"), req, resp)
+}
+```
+
+`TestStep`:
+
+```go
+{
+	ResourceName:                         resourceName,
+	ImportState:                          true,
+	ImportStateIdFunc:                    testAccVPCEndpointPrivateDNSImportStateIdFunc(resourceName),
+	ImportStateVerify:                    true,
+	ImportStateVerifyIdentifierAttribute: "vpc_endpoint_id",
+},
+```
+
+`ImportStateIdFunc`:
+
+```go
+func testAccVPCEndpointPrivateDNSImportStateIdFunc(resourceName string) resource.ImportStateIdFunc {
+	return func(s *terraform.State) (string, error) {
+		rs, ok := s.RootModule().Resources[resourceName]
+		if !ok {
+			return "", fmt.Errorf("Not found: %s", resourceName)
+		}
+
+		return rs.Primary.Attributes["vpc_endpoint_id"], nil
+	}
+}
+```
+
+### Multi-part Identifier
+
+In this example, the resource identifier is a combination of the `function_name` and `qualifier` arguments.
+
+`ImportState` Method:
+
+```go
+func (r *resourceRuntimeManagementConfig) ImportState(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) {
+	parts, err := intflex.ExpandResourceId(req.ID, runtimeManagementConfigIDParts, true)
+	if err != nil {
+		resp.Diagnostics.AddError(
+			"Unexpected Import Identifier",
+			fmt.Sprintf("Expected import identifier with format: function_name,qualifier. Got: %q", req.ID),
+		)
+		return
+	}
+
+	resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("function_name"), parts[0])...)
+	resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("qualifier"), parts[1])...)
+}
+```
+
+`TestStep`:
+
+```go
+{
+	ResourceName:                         resourceName,
+	ImportState:                          true,
+	ImportStateIdFunc:                    testAccRuntimeManagementConfigImportStateIdFunc(resourceName),
+	ImportStateVerify:                    true,
+	ImportStateVerifyIdentifierAttribute: "function_name",
+},
+```
+
+`ImportStateIdFunc`:
+
+```go
+func testAccRuntimeManagementConfigImportStateIdFunc(resourceName string) resource.ImportStateIdFunc {
+	return func(s *terraform.State) (string, error) {
+		rs, ok := s.RootModule().Resources[resourceName]
+		if !ok {
+			return "", fmt.Errorf("Not found: %s", resourceName)
+		}
+
+		return fmt.Sprintf("%s,%s", rs.Primary.Attributes["function_name"], rs.Primary.Attributes["qualifier"]), nil
+	}
+}
+```

docs/unit-tests.md

@@ -1,8 +1,10 @@
 # Unit Tests
 
-Unlike acceptance tests, unit tests do not access AWS and are focused on a function (or method). Because of this, they are quick and cheap to run.
+Unlike acceptance tests, unit tests do not access AWS and are focused on a function or method.
+Because of this, they are quick and cheap to run.
 
-In designing a resource's implementation, isolate complex bits from AWS bits so that they can be tested through a unit test. We encourage more unit tests in the provider.
+In designing a resource's implementation, isolate complex bits from AWS bits so that they can be tested through a unit test.
+We encourage more unit tests in the provider.
 
 ## In context
 
@@ -14,23 +16,25 @@ To help place unit testing in context, here is an overview of the Terraform AWS
 
 ## What to test
 
-Utilitarian functions that carry out processing within the provider, that don't need contact with AWS, are candidates for unit testing. In specific, any moderately to very complex utilitarian function should have a corresponding unit test.
+Utilitarian functions that carry out processing within the provider and don't need contact with AWS are candidates for unit testing.
+Any moderate to very complex utilitarian function should have a corresponding unit test.
 
-Rather than being a burden, using unit tests often saves time (and money) during development because they can be run quickly and locally. In addition, rather than mentally processing all edge cases, you can use test cases to refine the function's behavior.
+Rather than being a burden, using unit tests often save time and money during development because they can be run quickly and locally.
+In addition, rather than mentally processing all edge cases, you can use test cases to refine a function's behavior.
 
-Cut and dry functions using well-used patterns, like typical flatteners and expanders (flex functions), don't need unit testing. However, if the flex functions are complex or intricate, they should be unit tested.
+Cut and dry functions using well-used patterns, like typical flatteners and expanders (flex functions) don't need unit testing.
+However, if the flex functions are complex or intricate, they should be unit tested.
 
 ## Where they go
 
-Unit tests can be placed in a test file with acceptance tests if they mainly relate to a single resource. However, if it makes sense to do so, functions and their unit tests can be moved to their files. Reasons you might split them from acceptance test files include length of the acceptance test files, or several resources using them.
-
-Where unit tests are included with acceptance tests in resource test files, they should be placed at the top of the file, before the first acceptance test.
+Unit tests can be placed in a test file with acceptance tests if they mainly relate to a single resource.
+If a function is used across resources or is significantly complex, the function and it's unit tests can be moved to a standalone file.
+If unit tests are included with resource acceptance tests, they should be placed at the top before the first acceptance test.
 
 ## Example
 
 This is an example of a unit test.
-
-Its name is prefixed with "Test" but not "TestAcc" like an acceptance test.
+Its name is prefixed with `Test` but not `TestAcc` like an acceptance test.
 
 ```go
 func TestExampleUnitTest(t *testing.T) {

mkdocs.yml

@@ -25,6 +25,7 @@ nav:
           - Bugs and Enhancements: bugs-and-enhancements.md
           - Documentation Changes: documentation-changes.md
       - Acceptance Tests: running-and-writing-acceptance-tests.md
+      - Unit Tests: unit-tests.md
       - Continuous Integration: continuous-integration.md
       - Changelog Process: changelog-process.md
       - Raising a Pull Request: raising-a-pull-request.md
@@ -38,6 +39,7 @@ nav:
       - Dependency Updates: dependency-updates.md
       - Design Decision Log: design-decision-log.md
       - Error Handling: error-handling.md
+      - ID Attributes: id-attributes.md
       - Makefile Cheat Sheet: makefile-cheat-sheet.md
       - Naming Standards: naming.md
       - Provider Design: provider-design.md