provider/aws: Re-implement api gateway parameter handling #7794

nicolai86 · 2016-07-25T06:44:38Z

This PR is a follow up of my original API Gateway PR, #4295

Now that GH-2143 is finally closed this PR does away with the ugly request_parameters_in_json and response_parameters_in_json hack.

E.g. before:

resource "aws_api_gateway_method" "test" {
  rest_api_id = "${aws_api_gateway_rest_api.test.id}"
  resource_id = "${aws_api_gateway_resource.test.id}"
  http_method = "GET"
  authorization = "NONE"

  request_models = {
    "application/json" = "Error"
  }

  request_parameters = <<PARAM 
  {
      "method.request.querystring.page": false
  }
PARAM
}

after

resource "aws_api_gateway_method" "test" {
  rest_api_id = "${aws_api_gateway_rest_api.test.id}"
  resource_id = "${aws_api_gateway_resource.test.id}"
  http_method = "GET"
  authorization = "NONE"

  request_models = {
    "application/json" = "Error"
  }

  request_parameters = {
      "method.request.querystring.page" = false
  }
}

radeksimko · 2016-07-25T07:33:14Z

Thanks for the PR @nicolai86 , this looks a lot cleaner 🚿 👍 😄

I think we should however keep the old field there for a few version as Deprecated with appropriate message, e.g. "Use field request_parameters instead" to make the upgrade smooth for users.

I know it means keeping the old expand helper there too - feel free to duplicate those functions and rename the existing ones to something like deprecatedExpand....

If you add ConflictsWith to both fields (the deprecated one + the new one) then you can expect only a single field to be submitted at a time - then you can have a simple condition inside create/update which will call appropriate expand helper.

nicolai86 · 2016-07-25T07:45:29Z

Thanks for pointing out ConflictsWith @radeksimko! I'll update the PR accordingly later today!

this PR cleans up some left overs from PR #4295, namely the parameter handling. now that GH-2143 is finally closed this PR does away with the ugly `request_parameters_in_json` and `response_parameters_in_json` hack.

@radeksimko

following @radeksimko s advice, keeping the old code around with a deprecation warning. this should be cleaned up in a few releases

nicolai86 · 2016-08-09T06:45:52Z

@radeksimko I've adjusted the PR to keep the old attribute with a deprecation warning around. Hope I did not missing anything

radeksimko · 2016-08-09T11:39:10Z

builtin/providers/aws/resource_aws_api_gateway_method.go

+		ops, err := deprecatedExpandApiGatewayMethodParametersJSONOperations(d, "request_parameters_in_json", "requestParameters")
+		if err != nil {
+			return err
+		}


I think you're missing operations = append(operations, ops...) here which is why tests are failing 😉
https://travis-ci.org/hashicorp/terraform/builds/150845813#L175

radeksimko · 2016-08-09T11:44:56Z

@nicolai86 Left you some comments there, I will have another look once Travis turns green 😃

nicolai86 · 2016-08-09T16:23:36Z

@radeksimko thanks again for the review. I've adjusted accordingly & tests are now green : )

radeksimko · 2016-08-10T10:40:43Z

builtin/providers/aws/resource_aws_api_gateway_method.go

+			parameters[k] = v.(bool)
+		}
+	}
+	if v, ok := d.GetOk("request_parameters"); ok {


Wasn't this supposed to be request_parameters_in_json here?

radeksimko · 2016-08-10T12:53:39Z

@nicolai86 Except that comment about wrong field name ^ there's one core schema bug affecting this PR unfortunately.

TypeMap values are always parsed as TypeString. This is why the current implementation will cause Terraform to crash when creating aws_api_gateway_method with request_parameters or aws_api_gateway_method_response with response_parameters.

See #8104

Good news is that we can get around it before it is actually fixed, e.g.

            parameters[k], ok = v.(bool)
            if !ok {
                value, _ := strconv.ParseBool(v.(string))
                parameters[k] = value
            }

Would you mind having a look at it?

@radeksimko

following @radeksimko s lead

nicolai86 · 2016-08-10T20:59:29Z

I'm always assuming that travis runs aws tests, too, but it doesn't. Ran the AWS tests manually, see below: I have some cloudwatch errors but I don't see an< in my account. is this broken for you too, @radeksimko ?

TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAPIGateway -timeout 120m
=== RUN   TestAccAWSAPIGatewayAccount_importBasic
--- PASS: TestAccAWSAPIGatewayAccount_importBasic (15.11s)
=== RUN   TestAccAWSAPIGatewayApiKey_importBasic
--- PASS: TestAccAWSAPIGatewayApiKey_importBasic (45.13s)
=== RUN   TestAccAWSAPIGatewayAccount_basic
--- FAIL: TestAccAWSAPIGatewayAccount_basic (19.07s)
    testing.go:264: Step 0 error: Error applying: 1 error(s) occurred:

        * aws_api_gateway_account.test: Updating API Gateway Account failed: BadRequestException: API Gateway could not successfully write to CloudWatch Logs using the ARN specified. This account may have reached a limit in CloudWatch Logs.
            status code: 400, request id: 2b031edf-5f3c-11e6-9186-d3e876570bdb
=== RUN   TestAccAWSAPIGatewayApiKey_basic
--- PASS: TestAccAWSAPIGatewayApiKey_basic (40.03s)
=== RUN   TestAccAWSAPIGatewayAuthorizer_basic
--- PASS: TestAccAWSAPIGatewayAuthorizer_basic (51.14s)
=== RUN   TestAccAWSAPIGatewayDeployment_basic
--- PASS: TestAccAWSAPIGatewayDeployment_basic (38.84s)
=== RUN   TestAccAWSAPIGatewayIntegrationResponse_basic
--- PASS: TestAccAWSAPIGatewayIntegrationResponse_basic (66.25s)
=== RUN   TestAccAWSAPIGatewayIntegration_basic
--- PASS: TestAccAWSAPIGatewayIntegration_basic (50.71s)
=== RUN   TestAccAWSAPIGatewayMethodResponse_basic
--- PASS: TestAccAWSAPIGatewayMethodResponse_basic (49.25s)
=== RUN   TestAccAWSAPIGatewayMethod_basic
--- PASS: TestAccAWSAPIGatewayMethod_basic (47.16s)
=== RUN   TestAccAWSAPIGatewayModel_basic
--- PASS: TestAccAWSAPIGatewayModel_basic (22.05s)
=== RUN   TestAccAWSAPIGatewayResource_basic
--- PASS: TestAccAWSAPIGatewayResource_basic (41.04s)
=== RUN   TestAccAWSAPIGatewayRestApi_basic
--- PASS: TestAccAWSAPIGatewayRestApi_basic (30.82s)
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/aws    516.612s
make: *** [testacc] Error 1

radeksimko · 2016-08-10T21:21:55Z

Thanks for the modifications.

I'm always assuming that travis runs aws tests, too, but it doesn't.

It does, but only once per day and only for master, it would be too slow & expensive to run per each PR.

This account may have reached a limit in CloudWatch Logs.

I will have a look tomorrow, but I reckon this would be just a local error to your AWS account... or eventual consistency.

radeksimko · 2016-08-11T10:49:55Z

LGTM.

All acceptance tests passing on my side:

TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAPIGateway -timeout 120m
=== RUN   TestAccAWSAPIGatewayAccount_importBasic
--- PASS: TestAccAWSAPIGatewayAccount_importBasic (12.62s)
=== RUN   TestAccAWSAPIGatewayApiKey_importBasic
--- PASS: TestAccAWSAPIGatewayApiKey_importBasic (30.86s)
=== RUN   TestAccAWSAPIGatewayAccount_basic
--- PASS: TestAccAWSAPIGatewayAccount_basic (98.31s)
=== RUN   TestAccAWSAPIGatewayApiKey_basic
--- PASS: TestAccAWSAPIGatewayApiKey_basic (31.46s)
=== RUN   TestAccAWSAPIGatewayAuthorizer_basic
--- PASS: TestAccAWSAPIGatewayAuthorizer_basic (36.03s)
=== RUN   TestAccAWSAPIGatewayDeployment_basic
--- PASS: TestAccAWSAPIGatewayDeployment_basic (26.67s)
=== RUN   TestAccAWSAPIGatewayIntegrationResponse_basic
--- PASS: TestAccAWSAPIGatewayIntegrationResponse_basic (41.56s)
=== RUN   TestAccAWSAPIGatewayIntegration_basic
--- PASS: TestAccAWSAPIGatewayIntegration_basic (35.52s)
=== RUN   TestAccAWSAPIGatewayMethodResponse_basic
--- PASS: TestAccAWSAPIGatewayMethodResponse_basic (36.47s)
=== RUN   TestAccAWSAPIGatewayMethod_basic
--- PASS: TestAccAWSAPIGatewayMethod_basic (31.39s)
=== RUN   TestAccAWSAPIGatewayModel_basic
--- PASS: TestAccAWSAPIGatewayModel_basic (43.17s)
=== RUN   TestAccAWSAPIGatewayResource_basic
--- PASS: TestAccAWSAPIGatewayResource_basic (33.35s)
=== RUN   TestAccAWSAPIGatewayRestApi_basic
--- PASS: TestAccAWSAPIGatewayRestApi_basic (30.32s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    487.756s

ghost · 2020-04-23T02:20:08Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

radeksimko added provider/aws enhancement labels Jul 25, 2016

radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Jul 25, 2016

radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Jul 25, 2016

radeksimko self-assigned this Jul 25, 2016

radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Jul 25, 2016

nicolai86 added 2 commits August 9, 2016 08:25

provider/aws: Re-implement api gateway parameter handling

eb548f1

this PR cleans up some left overs from PR #4295, namely the parameter handling. now that GH-2143 is finally closed this PR does away with the ugly `request_parameters_in_json` and `response_parameters_in_json` hack.

Add deprecation message and conflictsWith settings

96e664d

following @radeksimko s advice, keeping the old code around with a deprecation warning. this should be cleaned up in a few releases

radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Aug 9, 2016

radeksimko reviewed Aug 9, 2016
View reviewed changes

radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Aug 9, 2016

nicolai86 added 2 commits August 9, 2016 18:05

provider/aws: fix missing append operation

12e1db2

provider/aws: mark old parameters clearly as deprecated

eee8da1

apparentlymart removed the waiting-response An issue/pull request is waiting for a response from the community label Aug 9, 2016

radeksimko reviewed Aug 10, 2016
View reviewed changes

radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Aug 10, 2016

radeksimko mentioned this pull request Aug 10, 2016

core: Map values are always treated as strings (Elem is ignored) #8104

Closed

nicolai86 and others added 2 commits August 10, 2016 22:16

provider/aws work around #8104

6fd0e23

following @radeksimko s lead

provider/aws fix cnp error

b666210

radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Aug 11, 2016

radeksimko merged commit 66a14cb into hashicorp:master Aug 11, 2016

radeksimko added a commit that referenced this pull request Aug 11, 2016

aws/docs: Fix a few nitpicks after merging #7794

69f8a03

ghost locked and limited conversation to collaborators Apr 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws: Re-implement api gateway parameter handling #7794

provider/aws: Re-implement api gateway parameter handling #7794

nicolai86 commented Jul 25, 2016 •

edited

Loading

radeksimko commented Jul 25, 2016 •

edited

Loading

nicolai86 commented Jul 25, 2016

nicolai86 commented Aug 9, 2016

radeksimko Aug 9, 2016

radeksimko commented Aug 9, 2016

nicolai86 commented Aug 9, 2016

radeksimko Aug 10, 2016

radeksimko commented Aug 10, 2016 •

edited

Loading

nicolai86 commented Aug 10, 2016

radeksimko commented Aug 10, 2016 •

edited

Loading

radeksimko commented Aug 11, 2016

ghost commented Apr 23, 2020

provider/aws: Re-implement api gateway parameter handling #7794

provider/aws: Re-implement api gateway parameter handling #7794

Conversation

nicolai86 commented Jul 25, 2016 • edited Loading

radeksimko commented Jul 25, 2016 • edited Loading

nicolai86 commented Jul 25, 2016

nicolai86 commented Aug 9, 2016

radeksimko Aug 9, 2016

Choose a reason for hiding this comment

radeksimko commented Aug 9, 2016

nicolai86 commented Aug 9, 2016

radeksimko Aug 10, 2016

Choose a reason for hiding this comment

radeksimko commented Aug 10, 2016 • edited Loading

nicolai86 commented Aug 10, 2016

radeksimko commented Aug 10, 2016 • edited Loading

radeksimko commented Aug 11, 2016

ghost commented Apr 23, 2020

nicolai86 commented Jul 25, 2016 •

edited

Loading

radeksimko commented Jul 25, 2016 •

edited

Loading

radeksimko commented Aug 10, 2016 •

edited

Loading

radeksimko commented Aug 10, 2016 •

edited

Loading