Merge pull request #29005 from hashicorp/td-context-everywhere

Converts AWS API calls to and provider CRUD calls to `Context`-aware versions

Author: gdavison

.ci/.golangci.yml

@@ -9,5 +9,3 @@ linters:
 
 run:
   timeout: 10m
-  skip-dirs:
-    - .ci/providerlint/vendor

.ci/.golangci2.yml

@@ -106,5 +106,3 @@ linters-settings:
 
 run:
   timeout: 75m
-  skip-dirs:
-    - .ci/providerlint/vendor

.ci/.semgrep-caps-aws-ec2.yml

@@ -13,7 +13,9 @@ rules:
           metavariable: $NAME
           patterns:
             - pattern-regex: "(?i)AWS"
+      - focus-metavariable: $NAME
     severity: WARNING
+
   - id: aws-in-const-name
     languages:
       - go

.ci/.semgrep.yml

@@ -10,6 +10,7 @@ rules:
       - metavariable-regex:
           metavariable: "$FUNCNAME"
           regex: "^TestAcc[^_]+_([a-zA-Z]+[dD]isappears|[^_]+_disappears)$"
+      - focus-metavariable: $FUNCNAME
     severity: WARNING
 
   - id: aws-sdk-go-multiple-service-imports
@@ -869,16 +870,6 @@ rules:
       - pattern: "[]string{..., string($X), ...}"
     severity: WARNING
 
-  - id: avoid-context-TODO-in-tests
-    languages: [go]
-    message: Prefer using context.Background() instead of context.TODO()
-    paths:
-      include:
-        - "internal/**/*_test.go"
-    patterns:
-      - pattern: context.TODO
-    severity: ERROR
-
   - id: avoid-context-CRUD-handlers
     languages: [go]
     message: Prefer using WithoutTimeout CRUD handlers instead of Context variants

.ci/semgrep/acctest/naming/naming.yml

@@ -12,3 +12,31 @@ rules:
           regex: "^testAcc((?!Check)[a-zA-Z]+Destroy$|(Check)?Destroy)"
           # regex: "^testAccCheckDestroy"
     severity: WARNING
+
+  - id: destroy-check-with-provider
+    languages: [go]
+    message: The check destroy with provider function should match the pattern "testAccCheck<Resource>DestroyWithProvider".
+    paths:
+      include:
+        - "internal/**/*_test.go"
+    patterns:
+      - pattern: func $FUNCNAME(...) { ... }
+      - metavariable-regex:
+          metavariable: "$FUNCNAME"
+          regex: '^testAccCheck[a-zA-Z]+Destroy(?!With)Provider'
+    severity: WARNING
+
+  - id: destroy-check-signature
+    languages: [go]
+    message: The check destroy function should have the correct signature
+    paths:
+      include:
+        - "internal/**/*_test.go"
+    patterns:
+      - pattern: func $FUNCNAME(...) { ... }
+      - metavariable-regex:
+          metavariable: "$FUNCNAME"
+          regex: "^testAccCheck[a-zA-Z]+Destroy(?!WithProvider)"
+      - pattern-not: func $FUNCNAME(s *terraform.State) error { ... }
+      - pattern-not: func $FUNCNAME(...) resource.TestCheckFunc { ... }
+    severity: WARNING

.ci/semgrep/migrate/context.yml

@@ -0,0 +1,75 @@
+rules:
+  - id: aws-api-context
+    languages: [go]
+    message: All AWS API calls should use the WithContext version
+    paths: &paths
+      include:
+        - internal/service/*
+        - internal/acctest/*
+      exclude:
+        # `pattern-not` not working
+        - internal/service/kafkaconnect
+        # WIP
+        - internal/acctest/acctest.go
+    patterns:
+      - pattern: |
+          $CONN.$API(...)
+      - metavariable-regex:
+          metavariable: $CONN
+          regex: ^(?!conns)\w*([cC]onn)
+      - metavariable-regex:
+          metavariable: $API
+          # This weird construction is to get around greedy matching
+          regex: ^(?!.*WithContext).*$
+      - pattern-not: |
+          $CONN.$APIV2(ctx, ...)
+      - pattern-not: connect.$API(...)
+      - pattern-not: tfconnect.$API(...)
+      - pattern-not: codestarconnections.$API(...)
+      - pattern-not: tfcodestarconnections.$API(...)
+      - pattern-not: tfdirectconnect.$API(...)
+      - pattern-not: kafkaconnect.$API(...)
+      - pattern-not: tfkafkaconnect.$API(...)
+      - pattern-not: conn.Handlers.$X(...)
+      - pattern-not: conn.Handlers.$X.$Y(...)
+    severity: ERROR
+  - id: aws-helpers-context
+    languages: [go]
+    message: All AWS helper functions should take a context.Context
+    paths: *paths
+    patterns:
+      - pattern: |
+          $INNER(conn, ...)
+      - pattern-not: $V2(conn, ...)
+      - metavariable-regex:
+          metavariable: $V2
+          regex: ^New\w+Paginator$
+    severity: ERROR
+  - id: retry-context
+    languages: [go]
+    message: Waiter and retry functions should use the Context version
+    paths: *paths
+    patterns:
+      - pattern-either:
+          - pattern: $X.WaitForState()
+          - pattern: resource.Retry()
+          - pattern: tfresource.RetryWhen(...)
+          - pattern: tfresource.RetryWhenAWSErrCodeEquals(...)
+          - pattern: tfresource.RetryWhenAWSErrMessageContains(...)
+          - pattern: tfresource.RetryUntilNotFound(...)
+          - pattern: tfresource.RetryWhenNotFound(...)
+          - pattern: tfresource.RetryWhenNewResourceNotFound(...)
+          - pattern: tfresource.WaitUntil(...)
+    severity: WARNING
+  - id: context-todo
+    languages: [go]
+    message: Should not use `context.TODO()`
+    paths: *paths
+    pattern: context.TODO()
+    severity: ERROR
+  - id: schema-noop
+    languages: [go]
+    message: Should use `schema.NoopContext` instead of `schema.Noop`
+    paths: *paths
+    pattern: schema.Noop
+    severity: ERROR

GNUmakefile

@@ -174,8 +174,10 @@ gh-workflows-lint:
 
 golangci-lint:
 	@echo "==> Checking source code with golangci-lint..."
-	@golangci-lint run --config .ci/.golangci.yml ./$(PKG_NAME)/...
-	@golangci-lint run --config .ci/.golangci2.yml ./$(PKG_NAME)/...
+	@golangci-lint run \
+		--config .ci/.golangci.yml \
+		--config .ci/.golangci2.yml \
+		./$(PKG_NAME)/...
 
 providerlint:
 	@echo "==> Checking source code with providerlint..."
@@ -202,11 +204,11 @@ providerlint:
 		-XR005=false \
 		-XS001=false \
 		-XS002=false \
-		./$(PKG_NAME)/service/... ./$(PKG_NAME)/provider/...
+		./internal/service/... ./internal/provider/...
 
 importlint:
 	@echo "==> Checking source code with importlint..."
-	@impi --local . --scheme stdThirdPartyLocal ./$(PKG_NAME)/...
+	@impi --local . --scheme stdThirdPartyLocal ./internal/...
 
 tools:
 	cd .ci/providerlint && $(GO_VER) install .
@@ -263,6 +265,7 @@ semgrep:
 semall:
 	@echo "==> Running Semgrep checks locally (must have semgrep installed)..."
 	@semgrep --error --metrics=off \
+		$(if $(filter-out $(origin PKG), undefined),--include $(PKG_NAME),) \
 		--config .ci/.semgrep.yml \
 		--config .ci/.semgrep-caps-aws-ec2.yml \
 		--config .ci/.semgrep-configs.yml \
@@ -271,11 +274,12 @@ semall:
 		--config .ci/.semgrep-service-name2.yml \
 		--config .ci/.semgrep-service-name3.yml \
 		--config .ci/semgrep/acctest/ \
+		--config .ci/semgrep/migrate/ \
 		--config 'r/dgryski.semgrep-go.badnilguard' \
 		--config 'r/dgryski.semgrep-go.errnilcheck' \
-    	--config 'r/dgryski.semgrep-go.marshaljson' \
+		--config 'r/dgryski.semgrep-go.marshaljson' \
 		--config 'r/dgryski.semgrep-go.nilerr' \
-        --config 'r/dgryski.semgrep-go.oddifsequence' \
+		--config 'r/dgryski.semgrep-go.oddifsequence' \
 		--config 'r/dgryski.semgrep-go.oserrors'
 
 skaff:

docs/adding-a-tag-resource.md

@@ -20,19 +20,20 @@ import (
 )
 
 func TestAcc{Service}Tag_basic(t *testing.T) {
+	ctx := acctest.Context(t)
 	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
 	resourceName := "aws_{service}_tag.test"
 
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:                 func() { acctest.PreCheck(t) },
 		ErrorCheck:               acctest.ErrorCheck(t, {Service}.EndpointsID),
 		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
-		CheckDestroy:             testAccCheck{Service}TagDestroy,
+		CheckDestroy:             testAccCheck{Service}TagDestroy(ctx),
 		Steps: []resource.TestStep{
 			{
 				Config: testAcc{Service}TagConfig(rName, "key1", "value1"),
 				Check: resource.ComposeTestCheckFunc(
-					testAccCheck{Service}TagExists(resourceName),
+					testAccCheck{Service}TagExists(ctx, resourceName),
 					resource.TestCheckResourceAttr(resourceName, "key", "key1"),
 					resource.TestCheckResourceAttr(resourceName, "value", "value1"),
 				),
@@ -47,20 +48,21 @@ func TestAcc{Service}Tag_basic(t *testing.T) {
 }
 
 func TestAcc{Service}Tag_disappears(t *testing.T) {
+	ctx := acctest.Context(t)
 	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
 	resourceName := "aws_{service}_tag.test"
 
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:                 func() { acctest.PreCheck(t) },
 		ErrorCheck:               acctest.ErrorCheck(t, {Service}.EndpointsID),
 		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
-		CheckDestroy:             testAccCheck{Service}TagDestroy,
+		CheckDestroy:             testAccCheck{Service}TagDestroy(ctx),
 		Steps: []resource.TestStep{
 			{
 				Config: testAcc{Service}TagConfig(rName, "key1", "value1"),
 				Check: resource.ComposeTestCheckFunc(
-					testAccCheck{Service}TagExists(resourceName),
-					acctest.CheckResourceDisappears(acctest.Provider, resourceAws{Service}Tag(), resourceName),
+					testAccCheck{Service}TagExists(ctx, resourceName),
+					acctest.CheckResourceDisappears(ctx, acctest.Provider, resourceAws{Service}Tag(), resourceName),
 				),
 				ExpectNonEmptyPlan: true,
 			},
@@ -69,19 +71,20 @@ func TestAcc{Service}Tag_disappears(t *testing.T) {
 }
 
 func TestAcc{Service}Tag_Value(t *testing.T) {
+	ctx := acctest.Context(t)
 	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
 	resourceName := "aws_{service}_tag.test"
 
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck:                 func() { acctest.PreCheck(t) },
 		ErrorCheck:               acctest.ErrorCheck(t, {Service}.EndpointsID),
 		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
-		CheckDestroy:             testAccCheck{Service}TagDestroy,
+		CheckDestroy:             testAccCheck{Service}TagDestroy(ctx),
 		Steps: []resource.TestStep{
 			{
 				Config: testAcc{Service}TagConfig(rName, "key1", "value1"),
 				Check: resource.ComposeTestCheckFunc(
-					testAccCheck{Service}TagExists(resourceName),
+					testAccCheck{Service}TagExists(ctx, resourceName),
 					resource.TestCheckResourceAttr(resourceName, "key", "key1"),
 					resource.TestCheckResourceAttr(resourceName, "value", "value1"),
 				),
@@ -94,7 +97,7 @@ func TestAcc{Service}Tag_Value(t *testing.T) {
 			{
 				Config: testAcc{Service}TagConfig(rName, "key1", "value1updated"),
 				Check: resource.ComposeTestCheckFunc(
-					testAccCheck{Service}TagExists(resourceName),
+					testAccCheck{Service}TagExists(ctx, resourceName),
 					resource.TestCheckResourceAttr(resourceName, "key", "key1"),
 					resource.TestCheckResourceAttr(resourceName, "value", "value1updated"),
 				),