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.

Sign up for GitHub

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

Jump to bottom

[TechDebt]: Remove use of errs.Must within service packages #39456

Open
wants to merge 44 commits into
base: main
Choose a base branch
from
Open

[TechDebt]: Remove use of errs.Must within service packages #39456

wants to merge 44 commits into from
+476 −141

Conversation

jar-b
Copy link
Member

@jar-b jar-b commented Sep 23, 2024 •
edited
Loading

Description

This check will match on any use of errs.Must(...) within internal/service/**/. Errors should be handled explicitly in service packages to avoid the possibility of crashing the provider during execution. This function was originally intended for use while initializing service clients during the ConfigureProvider operation, where a panic is the appropriate course of action.

Relations

Closes #39038

Output from Acceptance Testing

% make testacc PKG=bedrockagent TESTS="TestAccBedrockAgent_serial/DataSource/basic|TestAccBedrockAgentAgentActionGroup_basic|TestAccBedrockAgentAgentAlias_basic|TestAccBedrockAgentAgentKnowledgeBaseAssociation_basic"
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.7 test ./internal/service/bedrockagent/... -v -count 1 -parallel 20 -run='TestAccBedrockAgent_serial/DataSource/basic|TestAccBedrockAgentAgentActionGroup_basic|TestAccBedrockAgentAgentAlias_basic|TestAccBedrockAgentAgentKnowledgeBaseAssociation_basic'  -timeout 360m

--- PASS: TestAccBedrockAgentAgentAlias_basic (33.15s)
--- PASS: TestAccBedrockAgentAgentActionGroup_basic (34.79s)
--- PASS: TestAccBedrockAgentAgentKnowledgeBaseAssociation_basic (1610.63s)
--- PASS: TestAccBedrockAgent_serial (1664.05s)
    --- PASS: TestAccBedrockAgent_serial/DataSource (1664.05s)
        --- PASS: TestAccBedrockAgent_serial/DataSource/basic (1664.05s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/bedrockagent       1669.423s
% make testacc PKG=cloudformation TESTS=TestAccCloudFormationStackInstances_basic
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.7 test ./internal/service/cloudformation/... -v -count 1 -parallel 20 -run='TestAccCloudFormationStackInstances_basic'  -timeout 360m

--- PASS: TestAccCloudFormationStackInstances_basic (81.52s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/cloudformation     88.170s
% make testacc PKG=cognitoidp TESTS=TestAccCognitoIDPUserPoolUICustomization_Client_image
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.7 test ./internal/service/cognitoidp/... -v -count 1 -parallel 20 -run='TestAccCognitoIDPUserPoolUICustomization_Client_image'  -timeout 360m

--- PASS: TestAccCognitoIDPUserPoolUICustomization_Client_image (31.82s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/cognitoidp 37.177s
% make testacc PKG=cloudfrontkeyvaluestore TESTS=TestAccCloudFrontKeyValueStoreKey_basic
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.7 test ./internal/service/cloudfrontkeyvaluestore/... -v -count 1 -parallel 20 -run='TestAccCloudFrontKeyValueStoreKey_basic'  -timeout 360m

--- PASS: TestAccCloudFrontKeyValueStoreKey_basic (110.98s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/cloudfrontkeyvaluestore    117.716s
% make testacc PKG=connect TESTS=TestAccConnect_serial/LambdaFunctionAssociation/basic
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.7 test ./internal/service/connect/... -v -count 1 -parallel 20 -run='TestAccConnect_serial/LambdaFunctionAssociation/basic'  -timeout 360m

--- PASS: TestAccConnect_serial (175.23s)
    --- PASS: TestAccConnect_serial/LambdaFunctionAssociation (175.23s)
        --- PASS: TestAccConnect_serial/LambdaFunctionAssociation/basic (89.25s)
        --- PASS: TestAccConnect_serial/LambdaFunctionAssociation/dataSource_basic (85.98s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/connect    181.961s
% make testacc PKG=ebs TESTS=TestAccEC2EBSFastSnapshotRestore_basic
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.7 test ./internal/service/ec2/... -v -count 1 -parallel 20 -run='TestAccEC2EBSFastSnapshotRestore_basic'  -timeout 360m

--- PASS: TestAccEC2EBSFastSnapshotRestore_basic (168.27s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/ec2        175.209s
% make testacc PKG=elbv2 TESTS=TestAccELBV2TrustStoreRevocation_basic
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.7 test ./internal/service/elbv2/... -v -count 1 -parallel 20 -run='TestAccELBV2TrustStoreRevocation_basic'  -timeout 360m

--- PASS: TestAccELBV2TrustStoreRevocation_basic (43.79s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/elbv2      49.271s
% make testacc PKG=elasticache TESTS=TestAccElastiCacheUserGroupAssociation_basic
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.7 test ./internal/service/elasticache/... -v -count 1 -parallel 20 -run='TestAccElastiCacheUserGroupAssociation_basic'  -timeout 360m
=== RUN   TestAccElastiCacheUserGroupAssociation_basic
=== PAUSE TestAccElastiCacheUserGroupAssociation_basic
=== CONT  TestAccElastiCacheUserGroupAssociation_basic
--- PASS: TestAccElastiCacheUserGroupAssociation_basic (257.08s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/elasticache        262.428s
% make testacc PKG=lambda TESTS="TestAccLambdaLayerVersionPermission_basic_byARN|TestAccLambdaLayerVersionPermission_basic_byName|TestAccLambdaProvisionedConcurrencyConfig_basic"
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.7 test ./internal/service/lambda/... -v -count 1 -parallel 20 -run='TestAccLambdaLayerVersionPermission_basic_byARN|TestAccLambdaLayerVersionPermission_basic_byName|TestAccLambdaProvisionedConcurrencyConfig_basic'  -timeout 360m

--- PASS: TestAccLambdaLayerVersionPermission_basic_byName (15.52s)
--- PASS: TestAccLambdaLayerVersionPermission_basic_byARN (19.29s)
--- PASS: TestAccLambdaProvisionedConcurrencyConfig_basic (134.42s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/lambda     139.738s
% make testacc PKG=m2 TESTS=TestAccM2Deployment_basic
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.7 test ./internal/service/m2/... -v -count 1 -parallel 20 -run='TestAccM2Deployment_basic'  -timeout 360m

--- PASS: TestAccM2Deployment_basic (2507.94s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/m2 2514.812s
% make testacc PKG=networkmonitor TESTS=TestAccNetworkMonitorProbe_basic
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.7 test ./internal/service/networkmonitor/... -v -count 1 -parallel 20 -run='TestAccNetworkMonitorProbe_basic'  -timeout 360m
=== RUN   TestAccNetworkMonitorProbe_basic
=== PAUSE TestAccNetworkMonitorProbe_basic
=== CONT  TestAccNetworkMonitorProbe_basic
    probe_test.go:34: Step 3/3 error: Error running apply: exit status 1

        Error: waiting for CloudWatch Network Monitor Probe (tf-acc-test-7273269475067989614,probe-5ujg03zhv8segr8lda074dezf) delete

        timeout while waiting for resource to be gone (last state: 'DELETING',
        timeout: 15m0s)
    panic.go:626: Error running post-test destroy, there may be dangling resources: exit status 1

        Error: deleting CloudWatch Network Monitor Probe (tf-acc-test-7273269475067989614,probe-5ujg03zhv8segr8lda074dezf)

        operation error NetworkMonitor: DeleteProbe, exceeded maximum number of
        attempts, 25, https response error StatusCode: 500, RequestID:
        843deb04-f53d-46e6-b3d1-583dae3cf4c8, InternalServerException: Unexpected
        error occurred.
--- FAIL: TestAccNetworkMonitorProbe_basic (4410.10s)
FAIL
FAIL    github.com/hashicorp/terraform-provider-aws/internal/service/networkmonitor     4416.932s

☝️ Error is pre-existing and occurring in CI.

% make testacc PKG=route53 TESTS="TestAccRoute53CIDRLocation_basic|TestAccRoute53KeySigningKey_basic"
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.7 test ./internal/service/route53/... -v -count 1 -parallel 20 -run='TestAccRoute53CIDRLocation_basic|TestAccRoute53KeySigningKey_basic'  -timeout 360m

--- PASS: TestAccRoute53CIDRLocation_basic (11.21s)
--- PASS: TestAccRoute53KeySigningKey_basic (205.49s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/route53    210.870s
% make testacc PKG=s3control TESTS="TestAccS3ControlAccessGrants_serial/Location/basic|TestAccS3ControlAccessGrants_serial/Grant/basic"
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.7 test ./internal/service/s3control/... -v -count 1 -parallel 20 -run='TestAccS3ControlAccessGrants_serial/Location/basic|TestAccS3ControlAccessGrants_serial/Grant/basic'  -timeout 360m

--- PASS: TestAccS3ControlAccessGrants_serial (48.42s)
    --- PASS: TestAccS3ControlAccessGrants_serial/Location (24.39s)
        --- PASS: TestAccS3ControlAccessGrants_serial/Location/basic (24.39s)
    --- PASS: TestAccS3ControlAccessGrants_serial/Grant (24.03s)
        --- PASS: TestAccS3ControlAccessGrants_serial/Grant/basic (24.03s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/s3control  55.084s
% make testacc PKG=ssm TESTS=TestAccSSMPatchGroup_basic
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.7 test ./internal/service/ssm/... -v -count 1 -parallel 20 -run='TestAccSSMPatchGroup_basic'  -timeout 360m

--- PASS: TestAccSSMPatchGroup_basic (8.71s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/ssm        14.115s
% make testacc PKG=wafv2 TESTS=TestAccWAFV2WebACLAssociation_basic
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.22.7 test ./internal/service/wafv2/... -v -count 1 -parallel 20 -run='TestAccWAFV2WebACLAssociation_basic'  -timeout 360m

--- PASS: TestAccWAFV2WebACLAssociation_basic (46.10s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/wafv2      51.404s

@github-actions GitHub Actions
Copy link

Community Note

Voting for Prioritization

  • Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
  • Please see our prioritization guide for information on how we prioritize.
  • 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.

For Submitters

  • Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
  • For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
  • Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

@github-actions github-actions bot added tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. service/lambda Issues and PRs that pertain to the lambda service. service/route53 Issues and PRs that pertain to the route53 service. service/elbv2 Issues and PRs that pertain to the elbv2 service. service/dynamodb Issues and PRs that pertain to the dynamodb service. service/ssm Issues and PRs that pertain to the ssm service. service/appsync Issues and PRs that pertain to the appsync service. service/elasticache Issues and PRs that pertain to the elasticache service. service/cloudformation Issues and PRs that pertain to the cloudformation service. service/route53domains Issues and PRs that pertain to the route53domains service. service/fms Issues and PRs that pertain to the fms service. service/acmpca Issues and PRs that pertain to the acmpca service. service/connect Issues and PRs that pertain to the connect service. service/s3control Issues and PRs that pertain to the s3control service. service/ram Issues and PRs that pertain to the ram service. service/securityhub Issues and PRs that pertain to the securityhub service. service/wafv2 Issues and PRs that pertain to the wafv2 service. service/computeoptimizer Issues and PRs that pertain to the computeoptimizer service. linter Pertains to changes to or issues with the various linters. sweeper Pertains to changes to or issues with the sweeper. service/account Issues and PRs that pertain to the account service. service/grafana Issues and PRs that pertain to the grafana service. service/cognitoidp Issues and PRs that pertain to the cognitoidp service. service/ec2ebs Issues and PRs that pertain to the ec2ebs service. service/redshiftserverless Issues and PRs that pertain to the redshiftserverless service. service/controltower Issues and PRs that pertain to the controltower service. service/securitylake Issues and PRs that pertain to the securitylake service. service/lexv2models Issues and PRs that pertain to the lexv2models service. service/appfabric Issues and PRs that pertain to the appfabric service. labels Sep 23, 2024
@jar-b jar-b marked this pull request as ready for review September 24, 2024 17:48
@jar-b jar-b requested a review from a team as a code owner September 24, 2024 17:48
@jar-b
account: omit id from error if FlattenResourceId fails
27f6e6b
@jar-b
acmpca: omit id from error if FlattenResourceId fails
478750e
@jar-b
cognitoidp: omit id from error if FlattenResourceId fails
f92a3c2
@jar-b
connect: omit id from error if FlattenResourceId fails
a9b4cbd
@jar-b
controltower: omit id from error if FlattenResourceId fails
8500471
@jar-b
dynamodb: omit id from error if FlattenResourceId fails
8872350
@jar-b
elasticache: omit id from error if FlattenResourceId fails
c1e82a6
@jar-b
lambda: omit id from error if FlattenResourceId fails
fcb8dac
@jar-b
ram: omit id from error if FlattenResourceId fails
597e5b6
@jar-b
route53: omit id from error if FlattenResourceId fails
578ff16
@jar-b
securityhub: omit id from error if FlattenResourceId fails
e5996f3
@jar-b
ssm: omit id from error if FlattenResourceId fails
4dcade9
@jar-b
wafv2: omit id from error if FlattenResourceId fails
2ead23c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ewbankkit ewbankkit ewbankkit left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
linter Pertains to changes to or issues with the various linters. prioritized Part of the maintainer teams immediate focus. To be addressed within the current quarter. service/account Issues and PRs that pertain to the account service. service/acmpca Issues and PRs that pertain to the acmpca service. service/appfabric Issues and PRs that pertain to the appfabric service. service/appsync Issues and PRs that pertain to the appsync service. service/bedrockagent Issues and PRs that pertain to the bedrockagent service. service/cloudformation Issues and PRs that pertain to the cloudformation service. service/cloudfrontkeyvaluestore Issues and PRs that pertain to the cloudfrontkeyvaluestore service. service/cognitoidp Issues and PRs that pertain to the cognitoidp service. service/computeoptimizer Issues and PRs that pertain to the computeoptimizer service. service/connect Issues and PRs that pertain to the connect service. service/controltower Issues and PRs that pertain to the controltower service. service/dynamodb Issues and PRs that pertain to the dynamodb service. service/ec2ebs Issues and PRs that pertain to the ec2ebs service. service/elasticache Issues and PRs that pertain to the elasticache service. service/elbv2 Issues and PRs that pertain to the elbv2 service. service/fms Issues and PRs that pertain to the fms service. service/grafana Issues and PRs that pertain to the grafana service. service/lambda Issues and PRs that pertain to the lambda service. service/lexv2models Issues and PRs that pertain to the lexv2models service. service/m2 Issues and PRs that pertain to the m2 service. service/networkmonitor Issues and PRs that pertain to the networkmonitor service. service/ram Issues and PRs that pertain to the ram service. service/redshiftserverless Issues and PRs that pertain to the redshiftserverless service. service/route53domains Issues and PRs that pertain to the route53domains service. service/route53 Issues and PRs that pertain to the route53 service. service/s3control Issues and PRs that pertain to the s3control service. service/securityhub Issues and PRs that pertain to the securityhub service. service/securitylake Issues and PRs that pertain to the securitylake service. service/ssm Issues and PRs that pertain to the ssm service. service/wafv2 Issues and PRs that pertain to the wafv2 service. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TechDebt]: Reduce use of errs.Must
2 participants
@jar-b @ewbankkit