r/aws_elasticache_replication_group: Add a DiffSuppressFunc to the en…

[shuheiktgw]

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" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #15625

Release note for CHANGELOG:

resource/aws_elasticache_replication_group: Add a DiffSuppressFunc to the engine_version field

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSElasticacheReplicationGroup_redisEngineVersion6x''
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSElasticacheReplicationGroup_redisEngineVersion6x -timeout 120m
=== RUN   TestAccAWSElasticacheReplicationGroup_redisEngineVersion6x
=== PAUSE TestAccAWSElasticacheReplicationGroup_redisEngineVersion6x
=== CONT  TestAccAWSElasticacheReplicationGroup_redisEngineVersion6x
--- PASS: TestAccAWSElasticacheReplicationGroup_redisEngineVersion6x (643.37s)
PASS

Added a diff suppression function for the engine_version field. Thank you for your review!

[ktham]

What if someone created Redis with "5.0.5" and then later changed it to "5.x"? I wouldn't expect someone to actually do that, but if someone did, rather than getting an expected apply-time error, your DiffSuppressFunc might actually cause TF ignore that change if I follow your code correctly.

I think what you want to do is to compare the old major version and the new major version, check that it's >= 6, then return true if the both major versions are equal and the new minor version == "x".

I.e. We want to suppress changes for6.0.5 -> 6.x - we'd check that the major versions are equal, the major version is >=6, and new minor version == "x".

However, TF should NOT suppress changes for 5.0.5 -> 5.x, the expectation is that this should lead to an apply-time error

[demolitionmode]

@shuheiktgw Awesome work on the fix, please could you address #15702 (comment)

[shuheiktgw]

@ktham @demolitionmode Sorry I haven't noticed your comments for so long! I've fixed it now!

[ktham]

Awesome, thank you! @gdavison when you get a chance, can you review?

[demolitionmode]

@gdavison do you know a rough lead time for reviewing this please?
Also, @shuheiktgw it looks like there are now conflicts with the main branch that you may have to resolve.

[jseiser]

What needs done to push this over the line?

[ktham]

@gdavison hate to ping/bother you again ☹️ , but as I'm seeing other PR's getting merged and no movement on this one for 6 months which fixes a bug that affects many of us using TF to manage Elasticache, is this going to be reviewed and merged anytime soon?

[mrudrara]

This needs to be prioritized

[gdavison]

Thanks for the PR, @shuheiktgw. We've had some discussions on the team about the best way to handle the new way AWS is handling Redis versions for ElastiCache. We've decided that the best approach in this case is to have separate fields for the requested version (engine_version) and the running version (engine_version_actual). I've implemented this in PR #18920.

In addition, this makes the behaviour consistent across aws_elasticache_cluster and aws_elasticache_replication_group: aws_elasticache_replication_group now also allows version downgrades by re-creating the replication group, aws_elasticache_cluster also supports version 6.x for Redis. aws_elasticache_global_replication_group is also updated to match the engine_version_actual parameter.

Thanks again!

[ghost]

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!