New Resource: aws_api_gateway_documentation_version #3287

radeksimko · 2018-02-08T07:22:49Z

Test results

=== RUN   TestAccAWSAPIGatewayDocumentationVersion_basic
--- PASS: TestAccAWSAPIGatewayDocumentationVersion_basic (45.77s)
=== RUN   TestAccAWSAPIGatewayDocumentationVersion_allFields
--- PASS: TestAccAWSAPIGatewayDocumentationVersion_allFields (108.51s)
=== RUN   TestAccAWSAPIGatewayDocumentationVersion_importBasic
--- PASS: TestAccAWSAPIGatewayDocumentationVersion_importBasic (50.27s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	204.595s

bflad

As usual in pretty good shape. 👍 Please let me know if you have any questions! I can approve this once at least the version item is fixed up.

 make testacc TEST=./aws TESTARGS='-run=TestAccAWSAPIGatewayDocumentationVersion'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAPIGatewayDocumentationVersion -timeout 120m
=== RUN   TestAccAWSAPIGatewayDocumentationVersion_basic
--- PASS: TestAccAWSAPIGatewayDocumentationVersion_basic (15.29s)
=== RUN   TestAccAWSAPIGatewayDocumentationVersion_allFields
--- PASS: TestAccAWSAPIGatewayDocumentationVersion_allFields (53.44s)
=== RUN   TestAccAWSAPIGatewayDocumentationVersion_importBasic
--- PASS: TestAccAWSAPIGatewayDocumentationVersion_importBasic (16.14s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	84.923s

bflad · 2018-02-08T17:27:37Z

aws/resource_aws_api_gateway_documentation_version.go

+
+	restApiId := d.Get("rest_api_id").(string)
+
+	params := &apigateway.CreateDocumentationVersionInput{


Question: the CreateDocumentationVersionInput type also supports StageName, do we need to support it as well?

// The stage name to be associated with the new documentation snapshot. StageName *string `locationName:"stageName" type:"string"`

I intentionally removed it, although it was in my original implementation. CloudFormation also doesn't have it, probably for the same reason https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-apigateway-documentationversion.html

It has a very problematic workflow. You need to create the stage 1st (for referencing in APIG Documentation Version), but you cannot delete the APIG Documentation Version before deleting the stage (it errors out otherwise). Basically the dependency chain reverts during destruction.

Either way we shouldn't need it, because https://www.terraform.io/docs/providers/aws/r/api_gateway_stage.html#documentation_version allows users to associate the two.

👍 sounds good

bflad · 2018-02-08T17:30:03Z

aws/resource_aws_api_gateway_documentation_version.go

+	d.Set("version", docVersion)
+	d.Set("rest_api_id", apiId)
+	d.Set("description", version.Description)
+	d.Set("version", version.Version)


Setting version twice 😉

bflad · 2018-02-08T17:34:13Z

aws/resource_aws_api_gateway_documentation_version_test.go

+	})
+}
+
+func TestAccAWSAPIGatewayDocumentationVersion_importBasic(t *testing.T) {


Nitpick: Something I've been trying to remember to do is include ImportState TestSteps with each of the regular tests. We get the extra benefit of checking the additional attributes associated with each of the test cases and don't need explicit import test cases (which generally get neglected unless there's a 🐛 - too late!).

I have no objections against adding more import tests with different configurations, but I don't think it's a good idea to add ImportState to existing tests which aren't called _import.

If we break import (for any reason) and nothing else I expect _import tests to fail, but no other tests, and vice versa, ideally. While in reality this is may not be always true I think we should aim for such isolation so it's easier to debug test failures as they appear.

There's benefit to isolation, I think - to keep us sane and focused 😉

I added a new test - TestAccAWSAPIGatewayDocumentationVersion_importAllFields which should address this concern, hopefully? 😃

I don't mean to harp on this resource/test specifically, but here's where I'm coming from, using this resource's testing as an example. I'm not asking you to change this one, but I think its something worth thinking about. 😄

We now have two of these import tests, duplicating the regular ones except a new TestStep. This means the testing is going to take roughly twice as long. Not the biggest deal, but even running parallel these all add up. e.g.

make testacc TEST=./aws TESTARGS='-run=TestAccAWSAPIGatewayDocumentationVersion' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSAPIGatewayDocumentationVersion -timeout 120m === RUN TestAccAWSAPIGatewayDocumentationVersion_basic --- PASS: TestAccAWSAPIGatewayDocumentationVersion_basic (14.69s) === RUN TestAccAWSAPIGatewayDocumentationVersion_allFields --- PASS: TestAccAWSAPIGatewayDocumentationVersion_allFields (57.37s) === RUN TestAccAWSAPIGatewayDocumentationVersion_importBasic --- PASS: TestAccAWSAPIGatewayDocumentationVersion_importBasic (16.63s) === RUN TestAccAWSAPIGatewayDocumentationVersion_importAllFields --- PASS: TestAccAWSAPIGatewayDocumentationVersion_importAllFields (43.97s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 132.701s

The "regular" part (first TestStep) of these _importX tests can also fail because of other resources. There are plenty of examples of these in the EC2 or KMS daily acceptance testing. So if you're looking at just the test name and not the actual error message (we're looking at the error message right? 😉 ), you can be misled already.

I think TestStep.ImportStateVerify seems to do a pretty good job of calling out that its an import problem by the error message.

// Comment out in resourceAwsApiGatewayDocumentationVersionRead // d.Set("description", version.Description) // Add in resourceAwsApiGatewayDocumentationVersionCreate d.Set("description", d.Get("description").(string)) // Add to TestAccAWSAPIGatewayDocumentationVersion_allFields resource.TestStep{ ResourceName: resourceName, ImportState: true, ImportStateVerify: true, },

It returns:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSAPIGatewayDocumentationVersion_allFields' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSAPIGatewayDocumentationVersion_allFields -timeout 120m === RUN TestAccAWSAPIGatewayDocumentationVersion_allFields --- FAIL: TestAccAWSAPIGatewayDocumentationVersion_allFields (34.59s) testing.go:513: Step 2 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected. (map[string]string) { } (map[string]string) (len=1) { (string) (len=11) "description": (string) (len=40) "Tf Acc Test description updated xjsh6nsw" } FAIL exit status 1 FAIL github.com/terraform-providers/terraform-provider-aws/aws 34.628s

Combined with maybe a little bit better naming of the tests (e.g. _description in this case), we'll know from the test name its a description problem and from the error message its an import problem.

Hope this makes sense! 🍻 I guess mentally I am treating an attribute's import just a "feature" of an attribute, which should be logically tested with the other CRUD actions on it.

mentally I am treating an attribute's import just a "feature" of an attribute, which should be logically tested with the other CRUD actions on it.

That's an interesting POV. I never thought about that like this. We can discuss that on Slack with other folks later. 👌

bflad · 2018-02-08T17:36:17Z

aws/resource_aws_api_gateway_documentation_version.go

+	return err
+}
+
+func decodeApiGatewayDocVersionId(id string) (string, string, error) {


Worst OCD nitpick maybe of all time 😢 : Can you spell out Documentation so we can keep everything consistent with DocumentationVersion

bflad · 2018-02-08T17:38:39Z

aws/resource_aws_api_gateway_documentation_version_test.go

+  depends_on = ["aws_api_gateway_documentation_part.test"]
+}
+
+resource "aws_api_gateway_documentation_part" "test" {


Nitpick: Can we reuse something from the aws_api_gateway_documentation_part test configurations instead of duplicating this whole large configuration? 😄

I'm not sure it's a good idea as tests should ideally be as isolated as possible, at least cross resource, IMO.

bflad · 2018-02-08T17:40:22Z

website/docs/r/api_gateway_documentation_version.html.markdown

+
+# aws_api_gateway_documentation_version
+
+Provides a settings of an API Gateway Documentation Version.


Typo 🕶 : Maybe a resource to manage an here and slightly above?

radeksimko · 2018-02-08T18:50:05Z

=== RUN   TestAccAWSAPIGatewayDocumentationVersion_basic
--- PASS: TestAccAWSAPIGatewayDocumentationVersion_basic (46.78s)
=== RUN   TestAccAWSAPIGatewayDocumentationVersion_allFields
--- PASS: TestAccAWSAPIGatewayDocumentationVersion_allFields (97.67s)
=== RUN   TestAccAWSAPIGatewayDocumentationVersion_importBasic
--- PASS: TestAccAWSAPIGatewayDocumentationVersion_importBasic (44.24s)
=== RUN   TestAccAWSAPIGatewayDocumentationVersion_importAllFields
--- PASS: TestAccAWSAPIGatewayDocumentationVersion_importAllFields (61.75s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	250.464s

bflad

LGTM! (we can discuss my likely crazy opinions of import testing outside this PR)

make testacc TEST=./aws TESTARGS='-run=TestAccAWSAPIGatewayDocumentationVersion'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAPIGatewayDocumentationVersion -timeout 120m
=== RUN   TestAccAWSAPIGatewayDocumentationVersion_basic
--- PASS: TestAccAWSAPIGatewayDocumentationVersion_basic (14.69s)
=== RUN   TestAccAWSAPIGatewayDocumentationVersion_allFields
--- PASS: TestAccAWSAPIGatewayDocumentationVersion_allFields (57.37s)
=== RUN   TestAccAWSAPIGatewayDocumentationVersion_importBasic
--- PASS: TestAccAWSAPIGatewayDocumentationVersion_importBasic (16.63s)
=== RUN   TestAccAWSAPIGatewayDocumentationVersion_importAllFields
--- PASS: TestAccAWSAPIGatewayDocumentationVersion_importAllFields (43.97s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	132.701s

radeksimko · 2018-02-09T07:27:18Z

we can discuss my likely crazy opinions of import testing outside this PR

Those opinions are important and useful for a healthy discussion 😉 But yeah, let's discuss this outside of this PR.

bflad · 2018-02-09T22:27:11Z

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

ghost · 2020-04-08T17:21:02Z

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 new-resource Introduces a new resource. service/apigateway Issues and PRs that pertain to the apigateway service. labels Feb 8, 2018

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 8, 2018

bflad self-requested a review February 8, 2018 17:06

bflad reviewed Feb 8, 2018

View reviewed changes

New Resource: aws_api_gateway_documentation_version

eef28b1

radeksimko force-pushed the f-apig-doc-version branch from 10ea2fd to eef28b1 Compare February 8, 2018 18:49

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 8, 2018

bflad approved these changes Feb 8, 2018

View reviewed changes

radeksimko merged commit 82d266f into master Feb 9, 2018

radeksimko deleted the f-apig-doc-version branch February 9, 2018 07:27

radeksimko added this to the v1.9.0 milestone Feb 9, 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_version #3287

New Resource: aws_api_gateway_documentation_version #3287

radeksimko commented Feb 8, 2018

bflad left a comment

bflad Feb 8, 2018

radeksimko Feb 8, 2018 •

edited

Loading

bflad Feb 8, 2018

bflad Feb 8, 2018

bflad Feb 8, 2018

radeksimko Feb 8, 2018

radeksimko Feb 8, 2018

bflad Feb 8, 2018

radeksimko Feb 9, 2018

bflad Feb 8, 2018

bflad Feb 8, 2018

radeksimko Feb 8, 2018

bflad Feb 8, 2018

radeksimko commented Feb 8, 2018

bflad left a comment

radeksimko commented Feb 9, 2018

bflad commented Feb 9, 2018

ghost commented Apr 8, 2020


		restApiId := d.Get("rest_api_id").(string)

		params := &apigateway.CreateDocumentationVersionInput{


		# aws_api_gateway_documentation_version

		Provides a settings of an API Gateway Documentation Version.

New Resource: aws_api_gateway_documentation_version #3287

New Resource: aws_api_gateway_documentation_version #3287

Conversation

radeksimko commented Feb 8, 2018

Test results

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeksimko Feb 8, 2018 • edited Loading

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 Feb 8, 2018

bflad left a comment

Choose a reason for hiding this comment

radeksimko commented Feb 9, 2018

bflad commented Feb 9, 2018

ghost commented Apr 8, 2020

radeksimko Feb 8, 2018 •

edited

Loading