Added API Gateway Gateway response resource #1168

Ninir · 2017-07-17T12:53:09Z

Description

This adds the API Gateway Gateway response, which allows to configure.
This is a required step to complete the basic-auth protected HTTPs endpoint test to end #861.

Using this resource, we are now able to specify headers & response templates on a https status code basis, so per 401, 403, 500 errors.

Documentation: https://docs.aws.amazon.com/apigateway/api-reference/link-relation/gatewayresponse-update/

Tests

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSAPIGatewayGatewayResponse_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAPIGatewayGatewayResponse_ -timeout 120m
=== RUN   TestAccAWSAPIGatewayGatewayResponse_basic
--- PASS: TestAccAWSAPIGatewayGatewayResponse_basic (39.62s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	39.649s

TODOs

Add the resource + acceptance tests
Handle timeouts in the resource CRUD
Add the documentation

radeksimko

This looks overall ok, acceptance tests passing.

I pointed out a few small things. Just ping me when it's ready for another round.

radeksimko · 2017-07-18T11:43:31Z

website/docs/r/api_gateway_gateway_response.markdown

+
+# aws\_api\_gateway\_gateway\_response
+
+Provides an API Gateway Gateway Response for a REST API Gateway.


... Gateway Gateway ... Gateway 😂 🤷‍♂️

radeksimko · 2017-07-18T11:49:26Z

aws/resource_aws_api_gateway_gateway_response.go

+		RestApiId:          aws.String(d.Get("rest_api_id").(string)),
+		ResponseType:       aws.String(d.Get("response_type").(string)),
+		ResponseTemplates:  aws.StringMap(templates),
+		ResponseParameters: aws.StringMap(parameters),


I think if the two fields (response_templates & response_parameters) are optional and user didn't define those we shouldn't be sending them to the API here.

As discussed via Slack, this doesn't matter and we need it for updates, so 👍

radeksimko · 2017-07-18T11:51:40Z

aws/resource_aws_api_gateway_gateway_response.go

+	}
+
+	d.SetId(fmt.Sprintf("aggr-%s-%s", d.Get("rest_api_id").(string), d.Get("response_type").(string)))
+	log.Printf("[DEBUG] API Gateway Gateway Response ID: %s", d.Id())


Perhaps a nitpick, but more informative log message here would be for example

[DEBUG] API Gateway Gateway Response created (%q)

I think it's more obvious that the unique string is ID, but less obvious that this ID comes from creation (not update or deletion).

radeksimko · 2017-07-18T11:51:57Z

aws/resource_aws_api_gateway_gateway_response.go

+		if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "NotFoundException" {
+			log.Printf("[WARN] API Gateway Gateway Response (%s) not found, removing from state", d.Id())
+			d.SetId("")
+			return nil


radeksimko · 2017-07-18T11:53:35Z

aws/resource_aws_api_gateway_gateway_response.go

+	log.Printf("[DEBUG] Reading API Gateway Gateway Response %s", d.Id())
+	gatewayResponse, err := conn.GetGatewayResponse(&apigateway.GetGatewayResponseInput{
+		RestApiId:    aws.String(d.Get("rest_api_id").(string)),
+		ResponseType: aws.String(d.Get("response_type").(string)),


Since the combination of both rest_api_id and response_type is treated as "identifier" for the purpose of the lookup here shouldn't response_type also be ForceNew?

Yup, don't see any counter argument :)

radeksimko · 2017-07-18T12:05:41Z

aws/resource_aws_api_gateway_gateway_response.go

+			return resource.NonRetryableError(err)
+		}
+
+		if apigatewayErr.Code() == "TooManyRequestsException" {


This code is handled (and retried on) already in the AWS SDK: https://github.com/aws/aws-sdk-go/blob/master/aws/request/retryer.go#L41

If you think we don't retry enough times we should pass that feedback back to AWS SDK and deal with it there.

Didn't even know that :O Big win on that, thanks!

radeksimko · 2017-07-18T12:06:59Z

aws/resource_aws_api_gateway_gateway_response.go

+
+		if !ok {
+			return resource.NonRetryableError(err)
+		}


You can omit these three lines if you add ok && to the condition handling NotFoundException - it will just fall through to the last return. 😉

Good catch!

radeksimko · 2017-07-18T12:11:53Z

aws/resource_aws_api_gateway_gateway_response_test.go

+				),
+			},
+
+			resource.TestStep{


As in any slice/array of structs in Go this "annotation" is optional and we tend to omit it (less characters to read 😃 ) - like you did in the schema map.

https://play.golang.org/p/JcV3cTcxvn

radeksimko · 2017-07-18T12:12:47Z

aws/resource_aws_api_gateway_gateway_response_test.go

+		}
+
+		if rs.Primary.ID == "" {
+			return fmt.Errorf("No API Gateway Method ID is set")


radeksimko · 2017-07-18T12:13:47Z

aws/resource_aws_api_gateway_gateway_response_test.go

+		_, err := conn.GetGatewayResponse(req)
+
+		if err == nil {
+			return fmt.Errorf("API Gateway Method still exists")


radeksimko

I just left you two (last) comments, otherwise this is looking good. Thanks for fixing all the previous things.

radeksimko · 2017-07-18T17:20:01Z

aws/resource_aws_api_gateway_gateway_response.go

+	conn := meta.(*AWSClient).apigateway
+
+	templates := make(map[string]string)
+	for k, v := range d.Get("response_templates").(map[string]interface{}) {


Shouldn't this be also wrapped in GetOk() like the other optional field, as it's optional? Otherwise I think we might crash here when casting nil to a map.

Guhuhu, indeed it should have been set...!

radeksimko · 2017-07-18T17:21:07Z

aws/resource_aws_api_gateway_gateway_response.go

+		RestApiId:          aws.String(d.Get("rest_api_id").(string)),
+		ResponseType:       aws.String(d.Get("response_type").(string)),
+		ResponseTemplates:  aws.StringMap(templates),
+		ResponseParameters: aws.StringMap(parameters),


As discussed via Slack, this doesn't matter and we need it for updates, so 👍

radeksimko · 2017-07-18T17:24:02Z

aws/resource_aws_api_gateway_gateway_response.go

+
+	d.Set("response_type", gatewayResponse.ResponseType)
+	d.Set("status_code", gatewayResponse.StatusCode)
+	d.Set("response_templates", gatewayResponse.ResponseTemplates)


This comes back as map[string]*string, so shouldn't we also convert it to flat map via aws.StringValueMap like the other field below?

Good catch >_<"

radeksimko

LGTM 🎉

ghost · 2020-04-11T17:50:41Z

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!

Ninir force-pushed the f-apigw-gateway-responses branch 3 times, most recently from d2a0d15 to a6e1113 Compare July 18, 2017 07:28

Ninir changed the title ~~[WIP] Added API Gateway Gateway response resource~~ Added API Gateway Gateway response resource Jul 18, 2017

Ninir requested a review from radeksimko July 18, 2017 07:29

Ninir added the new-resource Introduces a new resource. label Jul 18, 2017

Ninir force-pushed the f-apigw-gateway-responses branch from a6e1113 to 7daeb71 Compare July 18, 2017 08:27

radeksimko suggested changes Jul 18, 2017

View reviewed changes

Ninir force-pushed the f-apigw-gateway-responses branch from 7daeb71 to c613df6 Compare July 18, 2017 15:36

radeksimko reviewed Jul 18, 2017

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Jul 18, 2017

Added API Gateway Gateway response resource

86cd267

Ninir force-pushed the f-apigw-gateway-responses branch from c613df6 to 86cd267 Compare July 18, 2017 19:32

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Jul 18, 2017

radeksimko approved these changes Jul 18, 2017

View reviewed changes

Ninir merged commit ee59d89 into hashicorp:master Jul 18, 2017

Ninir deleted the f-apigw-gateway-responses branch July 18, 2017 21:29

ghost locked and limited conversation to collaborators Apr 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added API Gateway Gateway response resource #1168

Added API Gateway Gateway response resource #1168

Ninir commented Jul 17, 2017 •

edited

Loading

radeksimko left a comment

radeksimko Jul 18, 2017

radeksimko Jul 18, 2017

radeksimko Jul 18, 2017

radeksimko Jul 18, 2017

radeksimko Jul 18, 2017

radeksimko Jul 18, 2017

Ninir Jul 18, 2017

radeksimko Jul 18, 2017

Ninir Jul 18, 2017

radeksimko Jul 18, 2017

Ninir Jul 18, 2017

radeksimko Jul 18, 2017

radeksimko Jul 18, 2017

radeksimko Jul 18, 2017

radeksimko left a comment

radeksimko Jul 18, 2017

Ninir Jul 18, 2017

radeksimko Jul 18, 2017

radeksimko Jul 18, 2017

Ninir Jul 18, 2017

radeksimko left a comment •

edited

Loading

ghost commented Apr 11, 2020


		# aws\_api\_gateway\_gateway\_response

		Provides an API Gateway Gateway Response for a REST API Gateway.

Added API Gateway Gateway response resource #1168

Added API Gateway Gateway response resource #1168

Conversation

Ninir commented Jul 17, 2017 • edited Loading

Description

Tests

TODOs

radeksimko 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 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

radeksimko left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented Apr 11, 2020

Ninir commented Jul 17, 2017 •

edited

Loading

radeksimko left a comment •

edited

Loading