Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

provider/aws: Added API Gateway Usage Plan #12542

Merged
merged 3 commits into from
Mar 18, 2017
Merged

provider/aws: Added API Gateway Usage Plan #12542

merged 3 commits into from
Mar 18, 2017

Conversation

Ninir
Copy link
Contributor

@Ninir Ninir commented Mar 8, 2017 •
edited
Loading

Description

This adds the API Gateway usage plan, based on the following API: https://docs.aws.amazon.com/apigateway/api-reference/resource/usage-plan/

Whereas the creation is made of Go Struct, Update is based on PatchOperations.

The table below is based on the update documentation:

Updateable property path op = add op = replace op = remove
/apiStages Supported Not supported Supported
/description Not supported Supported Not supported
/name Not supported Supported Not supported
/productCode Supported Supported Supported
/quota Not supported Not supported Supported
/quota/limit Supported Supported Not supported
/quota/offset Supported Supported Not supported
/quota/period Supported Supported Not supported
/throttle Not supported Not supported Supported
/throttle/burstLimit Supported Supported Not supported
/throttle/rateLimit Supported Supported Not supported

Related issues

#8532

Tests

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAPIGatewayUsagePlan_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/03/17 22:12:34 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAPIGatewayUsagePlan_ -timeout 120m
=== RUN   TestAccAWSAPIGatewayUsagePlan_importBasic
--- PASS: TestAccAWSAPIGatewayUsagePlan_importBasic (23.09s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_basic
--- PASS: TestAccAWSAPIGatewayUsagePlan_basic (75.64s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_description
--- PASS: TestAccAWSAPIGatewayUsagePlan_description (80.43s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_productCode
--- PASS: TestAccAWSAPIGatewayUsagePlan_productCode (112.72s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_throttling
--- PASS: TestAccAWSAPIGatewayUsagePlan_throttling (73.21s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_quota
--- PASS: TestAccAWSAPIGatewayUsagePlan_quota (88.84s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_apiStages
--- PASS: TestAccAWSAPIGatewayUsagePlan_apiStages (87.40s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	541.386s

TODOs

  • Add the API Gateway Usage Plan resource
  • Handle updates of Usage plan parts
  • Add validation
  • Add the documentation
  • Add test cases
  • Add import tests
  • Run & Fix tests

@Ninir Ninir force-pushed the f-aws-apigw-usageplan branch 4 times, most recently from 27cbcb3 to eddab20 Compare March 15, 2017 09:55
@Ninir Ninir force-pushed the f-aws-apigw-usageplan branch 3 times, most recently from a842ef2 to dd7e49e Compare March 17, 2017 21:34
@Ninir Ninir changed the title [WIP] provider/aws: Added API Gateway Usage Plan provider/aws: Added API Gateway Usage Plan Mar 17, 2017
@Ninir
Copy link
Contributor Author

Ninir commented Mar 17, 2017

Hi @stack72!

Compiled & played a lot with it, adding, removing arguments, replacing data, etc.
I think this is ready to go 😄

Gauthier

stack72
stack72 reviewed Mar 18, 2017
View reviewed changes
Copy link
Contributor

@stack72 stack72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Ninir

Left a few small comments. Apart from that, this LGTM

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAPIGatewayUsagePlan_'                                                  ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/03/18 12:37:12 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAPIGatewayUsagePlan_ -timeout 120m
=== RUN   TestAccAWSAPIGatewayUsagePlan_importBasic
--- PASS: TestAccAWSAPIGatewayUsagePlan_importBasic (43.59s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_basic
--- PASS: TestAccAWSAPIGatewayUsagePlan_basic (97.42s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_description
--- PASS: TestAccAWSAPIGatewayUsagePlan_description (149.52s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_productCode
--- PASS: TestAccAWSAPIGatewayUsagePlan_productCode (151.22s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_throttling
--- PASS: TestAccAWSAPIGatewayUsagePlan_throttling (118.91s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_quota
--- PASS: TestAccAWSAPIGatewayUsagePlan_quota (124.36s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_apiStages
--- PASS: TestAccAWSAPIGatewayUsagePlan_apiStages (163.00s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	848.058s

builtin/providers/aws/resource_aws_api_gateway_usage_plan.go
settings := v.(*schema.Set).List()
q, ok := settings[0].(map[string]interface{})

if errors := validateApiGatewayUsagePlanQuotaSettings(q); len(errors) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why validate here rather than pre create?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean setting the ValidateFunc? Well, I need to figure out the combination of 2 fields, and don't know how to proceed better :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok i understand what you mean now - let's leave this as is

builtin/providers/aws/resource_aws_api_gateway_usage_plan.go Outdated

if up.ApiStages != nil {
config := flattenApiGatewayUsageApiStages(up.ApiStages)
d.Set("api_stages", config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like us to check that the setting of this to state doesn't throw an error. i.e.

if err := d.Set("api_stages", flattenApiGatewayUsageApiStages(up.ApiStages)); err != nil {
			return fmt.Errorf("[DEBUG] Error setting api_stages error: %#v", err)
		}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, nice catch, will do!

builtin/providers/aws/resource_aws_api_gateway_usage_plan.go Outdated

if up.Throttle != nil {
config := flattenApiGatewayUsagePlanThrottling(up.Throttle)
d.Set("throttle_settings", config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

builtin/providers/aws/resource_aws_api_gateway_usage_plan.go Outdated

if up.Quota != nil {
config := flattenApiGatewayUsagePlanQuota(up.Quota)
d.Set("quota_settings", config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

builtin/providers/aws/resource_aws_api_gateway_usage_plan.go Outdated
return fmt.Errorf("Error updating API Gateway Usage Plan: %s", err)
}

d.SetId(*up.Id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to reset the ID here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, seems like a leftover, will remove. Thanks for the catch!

@Ninir
Copy link
Contributor Author

Ninir commented Mar 18, 2017

@stack72 Hi Paul,

Fixed your points. Here are the new acc tests:

$ TF_LOG=DEBUG TF_LOG_PATH=terraform.log make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAPIGatewayUsagePlan_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/03/18 14:47:55 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAPIGatewayUsagePlan_ -timeout 120m
=== RUN   TestAccAWSAPIGatewayUsagePlan_importBasic
--- PASS: TestAccAWSAPIGatewayUsagePlan_importBasic (26.29s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_basic
--- PASS: TestAccAWSAPIGatewayUsagePlan_basic (104.66s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_description
--- PASS: TestAccAWSAPIGatewayUsagePlan_description (97.53s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_productCode
--- PASS: TestAccAWSAPIGatewayUsagePlan_productCode (88.78s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_throttling
--- PASS: TestAccAWSAPIGatewayUsagePlan_throttling (63.81s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_quota
--- PASS: TestAccAWSAPIGatewayUsagePlan_quota (63.94s)
=== RUN   TestAccAWSAPIGatewayUsagePlan_apiStages
--- PASS: TestAccAWSAPIGatewayUsagePlan_apiStages (84.74s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	529.772s

Thanks for the review :)

@stack72
Copy link
Contributor

stack72 commented Mar 18, 2017

LGTM now thanks :)

@stack72 stack72 merged commit 323f646 into hashicorp:master Mar 18, 2017
@stack72 stack72 mentioned this pull request Mar 18, 2017
Closed
@Ninir Ninir deleted the f-aws-apigw-usageplan branch March 18, 2017 14:50
@Ninir Ninir mentioned this pull request Mar 18, 2017
Merged
6 tasks
@stack72
Copy link
Contributor

stack72 commented Mar 20, 2017

@Ninir can you ping me on stack72 at hashicorp dot com :)

mbfrahry pushed a commit that referenced this pull request Mar 28, 2017
@Ninir @mbfrahry
provider/aws: Added API Gateway Usage Plan (#12542)
a5b8095 
* Added api_gateway_usage_plan

* Updated documentation

* Fixed AWS usage plan review points
@ghost
Copy link

ghost commented Apr 15, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stack72 stack72 stack72 left review comments

Assignees
No one assigned
Labels
enhancement provider/aws
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Ninir @stack72