-
Notifications
You must be signed in to change notification settings - Fork 9.7k
Description
Community Note
- Please vote on this issue by adding a 👍 reaction to the original issue 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 issue followers and do not help prioritize the request
- If you are interested in working on this issue or have submitted a pull request, please leave a comment
Relates #21306
Relates #20959
Relates #20431
Relates #19661
Relates #19347
Relates #17893
Relates #17849
Relates #12840
Description
Maintainers and community contributors face an unnecessary layer of difficulty maintaining the code base such as nonresponsive IDEs and the inability to see all project files on GitHub. To address these growing pains, we are planning significant code refactoring of the Terraform AWS Provider. The refactoring will position the provider for better maintainability now and into the future as AWS ever expands its offerings.
What's New
These are highlights of some of the changes.
1. Broken PRs
As with any significant refactor, prior-PRs will be broken. We apologize for the inconvenience and will help you as much as we can to fix them. However, we hope that the benefits of refactoring will far outweigh the inconvenience now.
Examples of changes PRs need:
#21303
Guide to Fixing PRs After Service Packages
2. Where did everything go?
Before, all code lived in the aws
directory. Now, you'll find most code under the internal
directory. Here's an example of where files have gone:
Before:
.
+-- aws
| +-- resource_aws_autoscaling_group.go
| +-- resource_aws_instance.go
| +-- resource_aws_vpc.go
...
After:
.
+-- internal
| +-- service
| +-- autoscaling
| +-- group.go
| +-- ec2
| +-- instance.go
| +-- vpc.go
...
3. Naming
The names of everything from variables to functions to files are being simplified and cleaned up:
- 99% of the time, you no longer include "AWS" in every name.
- Resource file names do not include "resource" (e.g.,
instance.go
is the instance resource file). Data source do include "data_source" at the end of the file name (e.g.,regions_data_source.go
). - You no longer add the service name (e.g.,
SSM
) to every variable, function, and resource in the service. The one exception is acceptance test names (looking through a list of 6000 tests, it is still handy to see the service).
Here are examples of some name changes:
What | Old Name | New Name |
---|---|---|
File | aws/resource_aws_ssm_patch_group.go |
internal/service/ssm/patch_group.go |
File | aws/data_source_aws_msk_cluster.go |
internal/service/kafka/cluster_data_source.go |
Resource | resourceAwsMqBroker() |
ResourceBroker() |
Data Source | dataSourceAwsLambdaFunction() |
DataSourceFunction() |
Function | resourceAwsDynamoDbGlobalTableStateRefreshFunc() |
resourceGlobalTableStateRefreshFunc() |
waiter Function |
waiter.AccountAssignmentCreated() |
waitAccountAssignmentCreated() |
Acceptance Test | TestAccDataSourceAWSSignerSigningJob_basic |
TestAccSignerSigningJobDataSource_basic |
Acceptance Test | TestAccAWSAcmCertificate_tags |
TestAccACMCertificate_tags |
4. acctest
Package
When writing acceptance tests, many functions you'll use are now in the acctest
package. (What was previously called acctest
, "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
, is now aliased to sdkacctest
.)
func TestAccEC2FlowLog_vpcID(t *testing.T) {
...
rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acctest.PreCheck(t) },
ErrorCheck: acctest.ErrorCheck(t, ec2.EndpointsID),
Providers: acctest.Providers,
CheckDestroy: testAccCheckFlowLogDestroy,
Steps: []resource.TestStep{
{
Config: testAccFlowLogConfig_VPCID(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckFlowLogExists(resourceName, &flowLog),
acctest.MatchResourceAttrRegionalARN(resourceName, "arn", "ec2", regexp.MustCompile(`vpc-flow-log/fl-.+`)),
resource.TestCheckResourceAttrPair(resourceName, "iam_role_arn", iamRoleResourceName, "arn"),
...
),
},
},
})
}
5. Test Packages
Acceptance tests must be in the {service}_test
package. For example, TestAccEC2FlowLog_vpcID
would be in the ec2_test
package. Some unit tests are not in the {service}_test
package. To clarify, any test file that imports acctest
must be in {service}_test
to avoid an import cycle.
One implication of this is that acceptance tests must import their corresponding non-test packages to use its functions or constants. Also, those functions and constants must be exported (i.e., capitalized).
For example, if a test file in internal/service/ec2_test
uses a function called superDuper()
defined in internal/service/ec2
:
- the test file must import
"github.com/terraform-providers/terraform-provider-aws/internal/service/ec2"
, aliased astfec2
, superDuper()
must be exported asSuperDuper()
, and- the test file must reference
SuperDuper()
using the import alias, i.e.,tfec2.SuperDuper()
.
6. Running Tests
Tests run a little differently. For example, to run an IAM test:
% TF_ACC=1 go test ./internal/service/iam -v -count 1 -parallel 20 -run='TestAccIAMRole_basic' -timeout=180m
7. Running Sweepers
Running sweepers requires the sweep
build tag:
% go test ./internal/sweep -v -tags=sweep -sweep="us-west-2" -sweep-allow-failures -timeout=1h
New or Affected Resource(s)
- all
Potential Terraform Configuration
From a practitioner perspective, the AWS Provider should work the same.