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

resource/aws_api_gateway_rest_api: Add support for content encoding #3642

Merged
merged 1 commit into from
Mar 13, 2018
Merged

resource/aws_api_gateway_rest_api: Add support for content encoding #3642

merged 1 commit into from
Mar 13, 2018

Conversation

appilon
Copy link
Contributor

@appilon appilon commented Mar 6, 2018 •
edited
Loading

Fix #3144

It appears there is a bug with the method GetOkExists which prevents it from being useful in this situation (can't detect set --> unset/disable in config). Regardless it was decided using a special value of -1 keeps the code to 1 variable, and makes the implementation quite clean.

The tests finally pass by manually waiting and then reading the apigw. I imagine there might be a more "official" way of getting around the eventual consistency. No eventual consistency issues

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 6, 2018
@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. service/apigateway Issues and PRs that pertain to the apigateway service. labels Mar 7, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 7, 2018
@appilon appilon changed the title [WIP] implement minimumCompressionSize for APIGW Enable Content Encoding for APIGW Mar 7, 2018
@ghost ghost added size/L Managed by automation to categorize the size of a PR. labels Mar 7, 2018
@radeksimko
Copy link
Member

however I found ResourceData could not distinguish set from unset (after a value has been set). I imagine with more internal knowledge there might have been a way to pull it off. Overall I think I prefer the flag being user driven anyways, the implementation feels simpler and the experience is much more similar to that of the AWS web console (where you toggle content_encoding_enabled and then set a minimum size). A minimum size of 0 is sane imo too it just means all content is compressed.

Generally speaking we always tend to stick to API structure and naming conventions if possible and avoid building any abstractions above it as we burnt our fingers by doing so in the past where AWS changes API in a way that it broke our assumptions about such abstractions.

We should be able to implement this without having to introduce a new field. Before d.GetOkExists was a thing I'd recommend using -1 as "unset" placeholder in this context, but we shouldn't even need to do that.

d.GetOkExists("minimum_compression_size") should do the job. I assume it may not be working as expected because the field has Default set? Either way we don't usually set inherent (like empty string for TypeString, zero for TypeInt etc.) defaults in the schema. Default is there to avoid diffs when the API sets some defaults automatically after creation, typically non-zero value for TypeInt.

Let me know if you need any help in debugging this.

@radeksimko radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Mar 8, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 8, 2018
@appilon
Copy link
Contributor Author

appilon commented Mar 8, 2018

@radeksimko When I switch my acceptance test to wait a few seconds and manually read the api, the data is validated (suspected eventual consistency)

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 8, 2018
@radeksimko radeksimko mentioned this pull request Mar 12, 2018
Closed
radeksimko
radeksimko reviewed Mar 12, 2018
View reviewed changes
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Overall this is looking good now 👍

Besides my inline comment I have two reservations:

  1. I tried removing the Sleep and ran the acceptance test a couple of times - couldn't reproduce the issue with eventual consistency. Maybe the API is eventually consistent but don't remember ever experiencing the problem in API Gateway. More importantly if we are really affected by this it should be solved in the CRUD, not just in the test.
  2. Do you mind also documenting this new field?

(1) I'd be ok with just leaving the sleep out initially and see if anything emerges either as a bug report or in our nightly acceptance tests. If you're able to reproduce it then I'd be keen to see the debug log. 😉

aws/resource_aws_api_gateway_rest_api.go Outdated
@@ -74,6 +82,10 @@ func resourceAwsApiGatewayRestApiCreate(d *schema.ResourceData, meta interface{}
params.BinaryMediaTypes = expandStringList(binaryMediaTypes.([]interface{}))
}

if minimumCompressionSize := d.Get("minimum_compression_size").(int); minimumCompressionSize > -1 {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Do you mind putting the assignment on a separate line above as we're not checking result of casting? (which is AFAIK the common and often only reasonable use-case for these if one-liners)

@ghost ghost added size/L Managed by automation to categorize the size of a PR. size/M Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Mar 12, 2018
@appilon
implement content encoding for APIGW
1bd5c95 
- implemented with special -1 value to disable
- implement tests
- update docs
@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Mar 12, 2018
radeksimko
radeksimko approved these changes Mar 13, 2018
View reviewed changes
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

LGTM, :shipit:

It's totally not a big deal, but for the future we tend to create separate tests for new fields and functionality like this and keep _basic tests basic 😉

@radeksimko radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Mar 13, 2018
@appilon appilon changed the title Enable Content Encoding for APIGW resource/aws_api_gateway_rest_api: Add support for content encoding Mar 13, 2018
@appilon appilon merged commit 47adb1c into hashicorp:master Mar 13, 2018
@bflad bflad added this to the v1.12.0 milestone Mar 23, 2018
@bflad
Copy link
Contributor

bflad commented Mar 23, 2018

This has been released in version 1.12.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

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

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

@radeksimko radeksimko radeksimko approved these changes

Assignees
No one assigned
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/apigateway Issues and PRs that pertain to the apigateway service. size/M Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
v1.12.0
Development

Successfully merging this pull request may close these issues.

[Feature Request] Support for Content Encoding in API Gateway
3 participants
@appilon @radeksimko @bflad