New Resource: aws_api_gateway_documentation_part #2893

radeksimko · 2018-01-08T15:50:52Z

Test results

$ make testacc TESTARGS='-run=TestAccAWSAPIGatewayDocumentationPart_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSAPIGatewayDocumentationPart_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSAPIGatewayDocumentationPart_basic
--- PASS: TestAccAWSAPIGatewayDocumentationPart_basic (107.73s)
=== RUN   TestAccAWSAPIGatewayDocumentationPart_method
--- PASS: TestAccAWSAPIGatewayDocumentationPart_method (60.31s)
=== RUN   TestAccAWSAPIGatewayDocumentationPart_responseHeader
--- PASS: TestAccAWSAPIGatewayDocumentationPart_responseHeader (61.64s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	229.724s

bflad · 2018-01-10T00:11:18Z

aws/resource_aws_api_gateway_documentation_part.go

+		RestApiId:           aws.String(d.Get("rest_api_id").(string)),
+	})
+	if err != nil {
+		if isAWSErr(err, "NotFoundException", "") {


Nitpick: Constant is available for this exception: apigateway.ErrCodeNotFoundException

bflad

This is getting there! Only must fix items are related to adding an import test and fixing the documentation for that functionality (which may or may not require adjustment of the resource ID).

Passes acceptance testing in its current form for me:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSAPIGatewayDocumentationPart'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAPIGatewayDocumentationPart -timeout 120m
=== RUN   TestAccAWSAPIGatewayDocumentationPart_basic
--- PASS: TestAccAWSAPIGatewayDocumentationPart_basic (25.19s)
=== RUN   TestAccAWSAPIGatewayDocumentationPart_method
--- PASS: TestAccAWSAPIGatewayDocumentationPart_method (41.56s)
=== RUN   TestAccAWSAPIGatewayDocumentationPart_responseHeader
--- PASS: TestAccAWSAPIGatewayDocumentationPart_responseHeader (24.67s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	91.477s

bflad · 2018-01-10T00:13:48Z

aws/resource_aws_api_gateway_documentation_part_test.go

+			resource.TestStep{
+				Config: testAccAWSAPIGatewayDocumentationPartConfig(apiName, properties),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSAPIGatewayDocumentationPartExists("aws_api_gateway_documentation_part.test", &conf),


Nitpick (personal): I've been finding it helpful to declare the resource names as a top level variable in the function so its easier to duplicate code across functions/resources. e.g. resourceName := "aws_api_gateway_documentation_part.test" - personal opinion though!

I'm ok with that, sounds reasonable, the only thing is that the majority of the tests we have in the codebase now doesn't follow this, so we might want to revisit the existing code, so other folks copy-pasting code know what's "preferred".

bflad · 2018-01-10T00:16:21Z

aws/resource_aws_api_gateway_documentation_part_test.go

+	var conf apigateway.DocumentationPart
+
+	rString := acctest.RandString(8)
+	apiName := fmt.Sprintf("tf_acc_api_%s", rString)


Generic question: should we try to include the test name (e.g. t.Name()) instead of the fairly diverse naming we have across resources for acceptance tests? I'm guessing it really depends on naming restrictions for some resources (max length, no underscores, etc).

I agree we should make it test-specific, but t.Name() is a bit tricky for the reasons you mentioned, so I'll come up with custom, static name.

bflad · 2018-01-10T00:18:02Z

aws/resource_aws_api_gateway_documentation_part.go

+		Delete: resourceAwsApiGatewayDocumentationPartDelete,
+
+		Importer: &schema.ResourceImporter{
+			State: schema.ImportStatePassthrough,


Must fix: missing acceptance test for import functionality

Good catch! Will add.

bflad · 2018-01-10T00:21:04Z

aws/resource_aws_api_gateway_documentation_part.go

+	if err != nil {
+		return err
+	}
+	d.SetId(*out.Id)


Does the documentation part ID need to be combined with the REST API ID to support import? e.g. fmt.Sprintf("%s:%s", d.Get("rest_api_id").(string), *.out.Id) and requisite idParts := strings.Split(d.Id(), ":")

Yep, good catch, I'll pick / as the divider though. It seems safer, given that both IDs are then used in a URL, divided by that character.

bflad · 2018-01-10T00:25:08Z

website/docs/r/api_gateway_documentation_part.html.markdown

+
+#### `location`
+
+* `method` - (Optional) The HTTP verb of a method. It is a valid field for the API entity types of `METHOD`, `PATH_PARAMETER`, `QUERY_PARAMETER`, `REQUEST_HEADER`, `REQUEST_BODY`, `RESPONSE`, `RESPONSE_HEADER`, and `RESPONSE_BODY`. The default value is `*` for any method.


Can we refer to the API Gateway documentation for this attribute and the ones below instead of listing these out?

bflad · 2018-01-10T00:27:12Z

website/docs/r/api_gateway_documentation_part.html.markdown

+
+## Import
+
+API Gateway documentation_parts can be imported using the word `api-gateway-documentation_part`, e.g.


Needs to be updated for (REST API ID and )documentation part ID depending on outcome of ID value

bflad · 2018-01-10T00:29:21Z

aws/resource_aws_api_gateway_documentation_part_test.go

+			return err
+		}
+
+		return fmt.Errorf("API Gateway Documentation Part %s/%s still exists.",


Should be updated based on ID discussion (or at least made a little clear what the second ID is). 😄

bflad · 2018-01-10T00:29:45Z

aws/resource_aws_api_gateway_documentation_part_test.go

+		}
+		_, err := conn.GetDocumentationPart(req)
+		if err != nil {
+			if isAWSErr(err, "NotFoundException", "") {


Same note about apigateway.ErrCodeNotFoundException constant

bflad · 2018-01-10T00:32:17Z

aws/resource_aws_api_gateway_documentation_part_test.go

+					testAccCheckAWSAPIGatewayDocumentationPartExists("aws_api_gateway_documentation_part.test", &conf),
+					resource.TestCheckResourceAttr("aws_api_gateway_documentation_part.test", "location.#", "1"),
+					resource.TestCheckResourceAttr("aws_api_gateway_documentation_part.test", "location.0.type", "API"),
+					resource.TestCheckResourceAttr("aws_api_gateway_documentation_part.test", "properties", `{"description":"Terraform Acceptance Test"}`),


Can this use strconv.Unquote(properties) instead?

Good point in principle, I will use strconv.Quote().

radeksimko · 2018-01-10T14:21:54Z

@bflad I just addressed all of your comments. PTAL.

=== RUN   TestAccAWSAPIGatewayDocumentationPart_basic
--- PASS: TestAccAWSAPIGatewayDocumentationPart_basic (107.81s)
=== RUN   TestAccAWSAPIGatewayDocumentationPart_method
--- PASS: TestAccAWSAPIGatewayDocumentationPart_method (113.47s)
=== RUN   TestAccAWSAPIGatewayDocumentationPart_responseHeader
--- PASS: TestAccAWSAPIGatewayDocumentationPart_responseHeader (116.51s)
=== RUN   TestAccAWSAPIGatewayDocumentationPart_importBasic
--- PASS: TestAccAWSAPIGatewayDocumentationPart_importBasic (70.33s)

bflad

LGTM! Thanks for the fixes.

make testacc TEST=./aws TESTARGS='-run=TestAccAWSAPIGatewayDocumentationPart'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAPIGatewayDocumentationPart -timeout 120m
=== RUN   TestAccAWSAPIGatewayDocumentationPart_basic
--- PASS: TestAccAWSAPIGatewayDocumentationPart_basic (25.01s)
=== RUN   TestAccAWSAPIGatewayDocumentationPart_method
--- PASS: TestAccAWSAPIGatewayDocumentationPart_method (35.23s)
=== RUN   TestAccAWSAPIGatewayDocumentationPart_responseHeader
--- PASS: TestAccAWSAPIGatewayDocumentationPart_responseHeader (27.14s)
=== RUN   TestAccAWSAPIGatewayDocumentationPart_importBasic
--- PASS: TestAccAWSAPIGatewayDocumentationPart_importBasic (23.45s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	110.870s

bflad · 2018-01-12T14:28:27Z

This has been released in terraform-provider-aws version 1.7.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-08T17:51:39Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

radeksimko added the new-resource Introduces a new resource. label Jan 8, 2018

New Resource: aws_api_gateway_documentation_part

16d2836

radeksimko force-pushed the f-apig-documentation-part branch from 4c888d0 to 16d2836 Compare January 8, 2018 15:51

bflad self-assigned this Jan 10, 2018

bflad reviewed Jan 10, 2018

View reviewed changes

bflad suggested changes Jan 10, 2018

View reviewed changes

bflad assigned radeksimko Jan 10, 2018

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jan 10, 2018

Address PR comments

f5f3e4d

radeksimko force-pushed the f-apig-documentation-part branch from a66b6c8 to f5f3e4d Compare January 10, 2018 14:22

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 10, 2018

bflad approved these changes Jan 10, 2018

View reviewed changes

bflad added the service/apigateway Issues and PRs that pertain to the apigateway service. label Jan 11, 2018

radeksimko merged commit 0434a5b into master Jan 11, 2018

radeksimko deleted the f-apig-documentation-part branch January 11, 2018 07:36

bflad added this to the v1.7.0 milestone Jan 12, 2018

ghost locked and limited conversation to collaborators Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_api_gateway_documentation_part #2893

New Resource: aws_api_gateway_documentation_part #2893

radeksimko commented Jan 8, 2018

bflad Jan 10, 2018

bflad left a comment

bflad Jan 10, 2018

radeksimko Jan 10, 2018

bflad Jan 10, 2018

radeksimko Jan 10, 2018

bflad Jan 10, 2018

radeksimko Jan 10, 2018

bflad Jan 10, 2018

radeksimko Jan 10, 2018

bflad Jan 10, 2018

bflad Jan 10, 2018

bflad Jan 10, 2018

bflad Jan 10, 2018

bflad Jan 10, 2018

radeksimko Jan 10, 2018

radeksimko commented Jan 10, 2018

bflad left a comment

bflad commented Jan 12, 2018

ghost commented Apr 8, 2020


		#### `location`

		* `method` - (Optional) The HTTP verb of a method. It is a valid field for the API entity types of `METHOD`, `PATH_PARAMETER`, `QUERY_PARAMETER`, `REQUEST_HEADER`, `REQUEST_BODY`, `RESPONSE`, `RESPONSE_HEADER`, and `RESPONSE_BODY`. The default value is `*` for any method.


		## Import

		API Gateway documentation_parts can be imported using the word `api-gateway-documentation_part`, e.g.

New Resource: aws_api_gateway_documentation_part #2893

New Resource: aws_api_gateway_documentation_part #2893

Conversation

radeksimko commented Jan 8, 2018

Test results

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeksimko commented Jan 10, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Jan 12, 2018

ghost commented Apr 8, 2020