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

chore!: Rename resource aws_appsync_api_cache #70

Conversation

taufort
Copy link
Contributor

@taufort taufort commented Jan 8, 2025

Description

The aws_appsync_api_cache resource is named "example", which is not adequate and should be renamed to "this".
I also took a bit of time to update the example to make it easier to be used on other AWS accounts and also to make it more customizable.

Motivation and Context

Fixes #69

Breaking Changes

We need to bump Terraform version to be able to use Terraform moved block.

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

@taufort taufort changed the title refactor!: Rename resource aws_appsync_api_cache chore!: Rename resource aws_appsync_api_cache Jan 8, 2025
@taufort
Copy link
Contributor Author

taufort commented Jan 8, 2025

I had to rename my PR's title from "refactor!" to "chore!" because the default authorized commit types in GitHub action amannn/action-semantic-pull-request only contain the following types:

  • fix
  • feat
  • docs
  • ci
  • chore

The GitHub action allows one to override this default list (see https://github.com/amannn/action-semantic-pull-request/blob/main/action.yml#L11), I think it would be appropriate to add refactor in this list, WDYT?

@taufort taufort force-pushed the refactor/rename-api-cache-resource branch 2 times, most recently from d87286f to 4058eb2 Compare January 8, 2025 11:57
Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

Please revert the changes in examples/complete.

migrations.tf Outdated Show resolved Hide resolved
examples/complete/main.tf Show resolved Hide resolved
@@ -112,7 +94,7 @@ module "appsync" {
authentication_type = "OPENID_CONNECT"

lambda_authorizer_config = {
authorizer_uri = "arn:aws:lambda:eu-west-1:835367859851:function:appsync_auth_1"
authorizer_uri = "arn:aws:lambda:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:function:appsync_auth_1"
Copy link
Member

Choose a reason for hiding this comment

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

Users could use this output lambda_function_arn_static if they create Lambda functions using that module.

In this example, we don't need to have any assumption on AWS region or AWS account being used.

Copy link
Contributor Author

@taufort taufort Jan 9, 2025

Choose a reason for hiding this comment

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

The lambda authorizer should be in the same account as the caller. If I use the hard-coded account ID 835367859851 to test the example on one of my AWS account, I get this error:

│ Error: updating AppSync GraphQL API (bntnbrrxdfcf3iqd5qpebe6ucu): operation error AppSync: UpdateGraphqlApi, https response error StatusCode: 400, RequestID: daa839ef-a9ba-47d7-b350-1b2adeb318e6, BadRequestException: Lambda Authorizer should be in the same account as caller
│ 
│   with module.appsync.aws_appsync_graphql_api.this[0],
│   on ../../main.tf line 9, in resource "aws_appsync_graphql_api" "this":
│    9: resource "aws_appsync_graphql_api" "this" {
│ 

In my humble opinion, it's a bad practice having this account ID hard coded everywhere in the code for contributions because any contributor that will wish to apply this example on her/his AWS account will have to update the code to make the example work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the AWS region, I have the same opinion as the Account ID, it should be carried by the AWS provider you're using and not hard coded directly in the Terraform code. I have not changed the AWS region used previously, which was Ireland (eu-west-1).

@taufort taufort force-pushed the refactor/rename-api-cache-resource branch 2 times, most recently from f7b9cfa to 2d85cd0 Compare January 9, 2025 13:36
@taufort taufort force-pushed the refactor/rename-api-cache-resource branch from 2d85cd0 to 8388ce4 Compare January 9, 2025 15:57
@taufort
Copy link
Contributor Author

taufort commented Jan 9, 2025

@antonbabenko For information, I'm seeing a constant Terraform drift when I Terraform apply the example several times in a row without any change in the code:

Terraform will perform the following actions:

  # module.appsync.aws_appsync_graphql_api.this[0] will be updated in-place
  ~ resource "aws_appsync_graphql_api" "this" {
        id                   = "y4wlqbyrsbhp5eba7evicz52t4"
        name                 = "pro-dinosaur"
        tags                 = {
            "Name" = "pro-dinosaur"
        }
        # (11 unchanged attributes hidden)

      + lambda_authorizer_config {
          + authorizer_result_ttl_in_seconds = 300
          + authorizer_uri                   = "arn:aws:lambda:eu-west-1:835367859851:function:appsync_auth_1"
        }

        # (6 unchanged blocks hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

There must be an issue with this Lambda authorizer block.

Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

LGTM, it works in a similar way to the rest of our modules.

I think lambda_authorizer_config is drifting because the authorizer Lambda function is missing the required permissions, but it is out of the scope of this PR.

@antonbabenko antonbabenko merged commit 4b7f0b1 into terraform-aws-modules:master Jan 9, 2025
2 checks passed
antonbabenko pushed a commit that referenced this pull request Jan 9, 2025
## [3.0.0](v2.6.0...v3.0.0) (2025-01-09)

### ⚠ BREAKING CHANGES

* Rename resource aws_appsync_api_cache (#70)

### Miscellaneous Chores

* Rename resource aws_appsync_api_cache ([#70](#70)) ([4b7f0b1](4b7f0b1))
@antonbabenko
Copy link
Member

This PR is included in version 3.0.0 🎉

@antonbabenko
Copy link
Member

By accident (with ! in the PR/commit message)... now this became a major release... ok :)

@taufort
Copy link
Contributor Author

taufort commented Jan 10, 2025

By accident (with ! in the PR/commit message)... now this became a major release... ok :)

Is it really? I thought we said this would end up with a new major release. I may be wrong but I think we are not backward compatible with the change of Terraform version in versions.tf.

@taufort taufort deleted the refactor/rename-api-cache-resource branch January 10, 2025 08:21
@antonbabenko
Copy link
Member

@taufort You are right! The change of the Terraform version requires to be major release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename resource aws_appsync_api_cache
3 participants