Skip to content

feat: Ignore GSI changes#141

Open
pablotp wants to merge 3 commits intocloudposse:mainfrom
pablotp:allow-ignoring-gsi-changes
Open

feat: Ignore GSI changes#141
pablotp wants to merge 3 commits intocloudposse:mainfrom
pablotp:allow-ignoring-gsi-changes

Conversation

@pablotp
Copy link

@pablotp pablotp commented May 12, 2025

what

  • Ignore the changes made to the global-secondary-index.

why

  • Changes made to the global_secondary_index.read|write_capacity by the autoscaler causes drift.

references

@pablotp pablotp requested review from a team as code owners May 12, 2025 11:40
@mergify mergify bot added the triage Needs triage label May 12, 2025
@pablotp
Copy link
Author

pablotp commented May 12, 2025

/terratest

@pablotp pablotp force-pushed the allow-ignoring-gsi-changes branch 4 times, most recently from 64f3805 to 20452fd Compare May 12, 2025 12:25
@pablotp pablotp changed the title feat: Allow ignoring GSI changes feat: Ignore GSI changes May 12, 2025
@pablotp pablotp force-pushed the allow-ignoring-gsi-changes branch from 20452fd to 50b50d9 Compare May 12, 2025 12:27
@RoseSecurity
Copy link

Until the provider addresses this bug, this situation appears fair. We might need to add a README note to document that setting this option means Terraform will ignore any changes to GSIs and require manual or automated application.

@pablotp
Copy link
Author

pablotp commented May 19, 2025

Until the provider addresses this bug, this situation appears fair. We might need to add a README note to document that setting this option means Terraform will ignore any changes to GSIs and require manual or automated application.

Sounds good 👍🏻
I've just pushed aa6c434
Please let me know whether or not it meets your expectations. I generated the README.d using make readme, and it added a couple of unrelated changes (mainly replacing <br> with <br/>).

@mergify
Copy link

mergify bot commented Jun 11, 2025

💥 This pull request now has conflicts. Could you fix it @pablotp? 🙏

@mergify mergify bot added the conflict This PR has conflicts label Jun 11, 2025
@pablotp pablotp force-pushed the allow-ignoring-gsi-changes branch from aa6c434 to 0d22a4c Compare June 11, 2025 13:17
@mergify mergify bot removed the conflict This PR has conflicts label Jun 11, 2025
@RoseSecurity
Copy link

/terratest

I can't think of any alternative solutions to this issue, but I'd love to get a second set of eyes from the team on this change.

@pablotp
Copy link
Author

pablotp commented Jul 7, 2025

I can't think of any alternative solutions to this issue, but I'd love to get a second set of eyes from the team on this change.

Right now, I can't either.
After using this solution for a bit, I must admit that it gets tricky when you need to make changes apart from read|write_capacity (e.g., new secondary indexes). Therefore, I feel like this issue needs a solution at the provider level hashicorp/terraform-provider-aws#671.

Copy link
Member

@nitrocode nitrocode left a comment

Choose a reason for hiding this comment

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

Seems reasonable

@mergify mergify bot removed the triage Needs triage label Feb 17, 2026
@aknysh
Copy link
Member

aknysh commented Feb 17, 2026

/terratest

@nitrocode
Copy link
Member

Only downside to migrating from global_secondary_index argument to the new aws_dynamodb_global_secondary_index resource is that it's experimental...

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/dynamodb_global_secondary_index

@nitrocode
Copy link
Member

@aknysh this might still be the best way to go until the new resource is no longer experimental. What do you think?

Also, it looks like tests are failing on this which doesn't seem to have to do with this PR

TestExamplesComplete 2026-02-17T22:21:32Z logger.go:66: │ Error: creating AWS DynamoDB Table (eg-test-second): operation error DynamoDB: CreateTable, https response error StatusCode: 400, RequestID: VLA6KTJ3UHHNUSSPLIOEO3SUJJVV4KQNSO5AEMVJF66Q9ASUAAJG, ResourceInUseException: Table already exists: eg-test-second

@aknysh aknysh added the minor New features that do not break anything label Feb 18, 2026
@aknysh
Copy link
Member

aknysh commented Feb 18, 2026

@aknysh this might still be the best way to go until the new resource is no longer experimental. What do you think?

Also, it looks like tests are failing on this which doesn't seem to have to do with this PR

TestExamplesComplete 2026-02-17T22:21:32Z logger.go:66: │ Error: creating AWS DynamoDB Table (eg-test-second): operation error DynamoDB: CreateTable, https response error StatusCode: 400, RequestID: VLA6KTJ3UHHNUSSPLIOEO3SUJJVV4KQNSO5AEMVJF66Q9ASUAAJG, ResourceInUseException: Table already exists: eg-test-second

@pablotp @nitrocode it seems like a solution, but I know it gets and will get tricky to maintain the secondary indexes once they are ignored.
Maybe we can redesign the module to use this new feature which is already released?
hashicorp/terraform-provider-aws#44999

If it's experimental, we can prob wait for it to became GA, in which case I'm OK with the cnanges in the PR

@pablotp
Copy link
Author

pablotp commented Feb 18, 2026

@aknysh this might still be the best way to go until the new resource is no longer experimental. What do you think?
Also, it looks like tests are failing on this which doesn't seem to have to do with this PR

TestExamplesComplete 2026-02-17T22:21:32Z logger.go:66: │ Error: creating AWS DynamoDB Table (eg-test-second): operation error DynamoDB: CreateTable, https response error StatusCode: 400, RequestID: VLA6KTJ3UHHNUSSPLIOEO3SUJJVV4KQNSO5AEMVJF66Q9ASUAAJG, ResourceInUseException: Table already exists: eg-test-second

@pablotp @nitrocode it seems like a solution, but I know it gets and will get tricky to maintain the secondary indexes once they are ignored. Maybe we can redesign the module to use this new feature which is already released? hashicorp/terraform-provider-aws#44999

If it's experimental, we can prob wait for it to became GA, in which case I'm OK with the cnanges in the PR

Apologies for not having cleaned up this PR. We attempted this, but the problem is what @aknysh has already pointed out ⤵️

will get tricky to maintain the secondary indexes once they are ignored.

Maybe we can redesign the module to use this new feature which is already released hashicorp/terraform-provider-aws#44999

This seems to be the way to go 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor New features that do not break anything

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants