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

Big Changes: AWS Provider Refactor #20000

Closed
YakDriver opened this issue Jun 29, 2021 · 2 comments · Fixed by #21306
Closed

Big Changes: AWS Provider Refactor #20000

YakDriver opened this issue Jun 29, 2021 · 2 comments · Fixed by #21306
Assignees
Labels
enhancement Requests to existing resources that expand the functionality or scope.
Milestone

Comments

@YakDriver
Copy link
Member

YakDriver commented Jun 29, 2021

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:

  1. the test file must import "github.com/terraform-providers/terraform-provider-aws/internal/service/ec2", aliased as tfec2,
  2. superDuper() must be exported as SuperDuper(), and
  3. 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.

References

@YakDriver YakDriver added the enhancement Requests to existing resources that expand the functionality or scope. label Jun 29, 2021
@YakDriver YakDriver changed the title [wip] this is it Yay for 20,000! Jun 29, 2021
@YakDriver YakDriver changed the title Yay for 20,000! Yay for 20000! Jun 29, 2021
@YakDriver YakDriver changed the title Yay for 20000! Refactor AWS Provider into Service Packages Oct 14, 2021
@YakDriver YakDriver self-assigned this Oct 14, 2021
@github-actions github-actions bot added this to the v3.64.0 milestone Oct 14, 2021
@breathingdust breathingdust pinned this issue Oct 15, 2021
@YakDriver YakDriver changed the title Refactor AWS Provider into Service Packages Big Changes: AWS Provider Refactor Oct 15, 2021
@github-actions
Copy link

github-actions bot commented Nov 4, 2021

This functionality has been released in v3.64.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!

@breathingdust breathingdust unpinned this issue Nov 17, 2021
Pharb added a commit to Pharb/terraform-provider-aws that referenced this issue Nov 23, 2021
breathingdust pushed a commit that referenced this issue Dec 10, 2021
…ment (#20857)

* tests/resource/aws_appsync_graphql_api: Update deprecated Providers to ProviderFactories

* resource/aws_appsync_graphql_api: Add `lambda_authorizer_config` argument (#20644)

Issue: #20644
API docs: https://docs.aws.amazon.com/appsync/latest/APIReference/API_LambdaAuthorizerConfig.html

Output from acceptance testing:
```
make testacc TESTARGS='-run=TestAccAWSAppsyncGraphqlApi_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSAppsyncGraphqlApi_ -timeout 180m
--- PASS: TestAccAWSAppsyncGraphqlApi_basic (147.72s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AdditionalAuthentication_AWSIAM (148.24s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AdditionalAuthentication_APIKey (149.20s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AdditionalAuthentication_OpenIDConnect (149.32s)
--- PASS: TestAccAWSAppsyncGraphqlApi_LogConfig (153.51s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AdditionalAuthentication_CognitoUserPools (173.33s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AdditionalAuthentication_AwsLambda (196.04s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AdditionalAuthentication_Multiple (204.38s)
--- PASS: TestAccAWSAppsyncGraphqlApi_XrayEnabled (214.18s)
--- PASS: TestAccAWSAppsyncGraphqlApi_Name (215.34s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_AwsLambda (221.80s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_AuthTTL (235.05s)
--- PASS: TestAccAWSAppsyncGraphqlApi_Tags (236.82s)
--- PASS: TestAccAWSAppsyncGraphqlApi_UserPoolConfig_AwsRegion (242.45s)
--- PASS: TestAccAWSAppsyncGraphqlApi_LogConfig_ExcludeVerboseContent (250.77s)
--- PASS: TestAccAWSAppsyncGraphqlApi_UserPoolConfig_DefaultAction (250.94s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_APIKey (116.05s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_OpenIDConnect (115.22s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_AmazonCognitoUserPools (125.39s)
--- PASS: TestAccAWSAppsyncGraphqlApi_disappears (65.90s)
--- PASS: TestAccAWSAppsyncGraphqlApi_LambdaAuthorizerConfig_IdentityValidationExpression (281.96s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType_AWSIAM (88.56s)
--- PASS: TestAccAWSAppsyncGraphqlApi_LambdaAuthorizerConfig_AuthorizerUri (294.85s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_IatTTL (158.58s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_Issuer (154.12s)
--- PASS: TestAccAWSAppsyncGraphqlApi_LogConfig_FieldLogLevel (312.61s)
--- PASS: TestAccAWSAppsyncGraphqlApi_Schema (143.80s)
--- PASS: TestAccAWSAppsyncGraphqlApi_AuthenticationType (126.84s)
--- PASS: TestAccAWSAppsyncGraphqlApi_OpenIDConnectConfig_ClientID (113.13s)
--- PASS: TestAccAWSAppsyncGraphqlApi_LambdaAuthorizerConfig_AuthorizerResultTtlInSeconds (356.98s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       359.589s
```

* resource/aws_appsync_graphql_api: Add changelog entry 20857.txt

* resource/aws_appsync_graphql_api: Terraform fmt in test config

* Revert "tests/resource/aws_appsync_graphql_api: Update deprecated Providers to ProviderFactories"

This reverts commit 1f981fc.

* tests/resource/aws_appsync_graphql_api: Changes from #20000 and #21400

* graphql_api: Re-order map alphabetically (review comment)
KaguraKagura added a commit to KaguraKagura/terraform-provider-aws that referenced this issue Jan 18, 2022
… into f-lightsail-container-service while resolving the conflicts introduced by the refactor in hashicorp#20000
@github-actions
Copy link

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 have found a problem that seems similar to this, 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 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant