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.

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

resource/aws_api_gateway_rest_api: suppressed plan diffs for policy #10986

Closed
wants to merge 1 commit into from
Closed

resource/aws_api_gateway_rest_api: suppressed plan diffs for policy #10986

wants to merge 1 commit into from

Conversation

sergei-ivanov
Copy link
Contributor

@sergei-ivanov sergei-ivanov commented Nov 23, 2019

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Closes #5549
Relates #10706

Release note for CHANGELOG:

Suppress spurious plan changes for `policy` attribute of `aws_api_gateway_rest_api`

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSAPIGatewayRestApi_*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -count 1 -parallel 20 -run=TestAccAWSAPIGatewayRestApi_* -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSAPIGatewayRestApi_basic
=== PAUSE TestAccAWSAPIGatewayRestApi_basic
=== RUN   TestAccAWSAPIGatewayRestApi_tags
=== PAUSE TestAccAWSAPIGatewayRestApi_tags
=== RUN   TestAccAWSAPIGatewayRestApi_disappears
=== PAUSE TestAccAWSAPIGatewayRestApi_disappears
=== RUN   TestAccAWSAPIGatewayRestApi_EndpointConfiguration
=== PAUSE TestAccAWSAPIGatewayRestApi_EndpointConfiguration
=== RUN   TestAccAWSAPIGatewayRestApi_EndpointConfiguration_Private
=== PAUSE TestAccAWSAPIGatewayRestApi_EndpointConfiguration_Private
=== RUN   TestAccAWSAPIGatewayRestApi_api_key_source
=== PAUSE TestAccAWSAPIGatewayRestApi_api_key_source
=== RUN   TestAccAWSAPIGatewayRestApi_policy
=== PAUSE TestAccAWSAPIGatewayRestApi_policy
=== RUN   TestAccAWSAPIGatewayRestApi_openapi
=== PAUSE TestAccAWSAPIGatewayRestApi_openapi
=== CONT  TestAccAWSAPIGatewayRestApi_basic
=== CONT  TestAccAWSAPIGatewayRestApi_api_key_source
=== CONT  TestAccAWSAPIGatewayRestApi_tags
=== CONT  TestAccAWSAPIGatewayRestApi_disappears
=== CONT  TestAccAWSAPIGatewayRestApi_openapi
=== CONT  TestAccAWSAPIGatewayRestApi_EndpointConfiguration
=== CONT  TestAccAWSAPIGatewayRestApi_EndpointConfiguration_Private
=== CONT  TestAccAWSAPIGatewayRestApi_policy
--- PASS: TestAccAWSAPIGatewayRestApi_disappears (27.07s)
--- PASS: TestAccAWSAPIGatewayRestApi_EndpointConfiguration_Private (78.84s)
--- PASS: TestAccAWSAPIGatewayRestApi_EndpointConfiguration (114.13s)
--- PASS: TestAccAWSAPIGatewayRestApi_policy (143.60s)
--- PASS: TestAccAWSAPIGatewayRestApi_basic (199.59s)
--- PASS: TestAccAWSAPIGatewayRestApi_tags (221.69s)
--- PASS: TestAccAWSAPIGatewayRestApi_api_key_source (244.16s)
--- PASS: TestAccAWSAPIGatewayRestApi_openapi (298.21s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	298.263s
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/flatmap	0.015s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags	0.005s [no tests to run]

@sergei-ivanov sergei-ivanov requested a review from a team November 23, 2019 03:14
@ghost ghost added size/M Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. service/apigateway Issues and PRs that pertain to the apigateway service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Nov 23, 2019
@sergei-ivanov
Copy link
Contributor Author

Can anyone from the team please review?
This PR supersedes #10706 as a backwards-compatible implementation based on diff suppression.

@sergei-ivanov
Copy link
Contributor Author

Anyone to review please?

@sergei-ivanov
Copy link
Contributor Author

Hi @bflad , sorry for tagging you directly. Can you please take a look at this change, because I keep rebasing it and nobody picks it up. Here I have reimplemented #10706 as a result of our earlier discussion. I hope it is better this way, because it does not introduce any breaking changes.

@Constantin07
Copy link

Any ETA when this PR can be reviewed by maintainers ? I need this feature in my project.

@walkafwalka
Copy link

Looking forward to having this reviewed and implemented. Thanks @sergei-ivanov.

@shederman
Copy link

Any ETA on this PR? Yet another "Terraform broken for non-toy API Gateway usage" bug. Getting the very strong feeling that we shouldn't be using Terraform for API Gateway.

@sergei-ivanov
Copy link
Contributor Author

Rebased on the latest upstream master and resolved the conflicts.

@DrFaust92
Copy link
Collaborator

Hey @sergei-ivanov, thanks for the contribution. resource policies seem to be a pain point in several other resources as well. to give better control and prevent issues such as you are proposing to solve in this PR the maintainer team introduced some guidelines for resource access policy: https://github.com/terraform-providers/terraform-provider-aws/blob/master/docs/contributing/contribution-checklists.md#adding-resource-policy-support.

best solution here would be to implement this policy as separate resource as proposed at #5834 and deprecated this field eventually.
would you like to take a look? let me know ill address this otherwise in the in the following few days.

@bflad
Copy link
Contributor

bflad commented Nov 17, 2020

Hi @sergei-ivanov 👋 Thank you for submitting this. As mentioned above, we are now preferring that Terraform AWS Provider handling of resource policies be done via separate _policy resources, which allows direct interpolation of the resource ARN values into the policy. Given that #13619 implements the new aws_api_gateway_rest_api_policy resource, I'm going to close this pull request. Thanks again for the contribution though.

@bflad bflad closed this Nov 17, 2020
@sergei-ivanov sergei-ivanov deleted the diff-suppress branch November 19, 2020 14:06
@ghost
Copy link

ghost commented Dec 17, 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 as resolved and limited conversation to collaborators Dec 17, 2020
@breathingdust breathingdust removed the needs-triage Waiting for first response or review from a maintainer. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service/apigateway Issues and PRs that pertain to the apigateway service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform constantly updates resource policy on API Gateway
7 participants