cloudposse / terraform-aws-elasticache-redis

Terraform module to provision an ElastiCache Redis Cluster
https://cloudposse.com/accelerate
Apache License 2.0
141 stars 246 forks source link

v0.41.3: Unable to apply new Parameter Group with existing resources #142

Closed syphernl closed 2 years ago

syphernl commented 2 years ago

Describe the Bug

The changes done in v0.41.3 (#141) causes issues with existing resources. Since it re-uses the same name Terraform will try to remove it. But since it is in use, it will fail:

│ Error: error deleting ElastiCache Parameter Group (PROJECT-staging-redis): InvalidCacheParameterGroupState: One or more cache clusters are still members of this parameter group PROJECT-staging-redis, so the group cannot be deleted.
│   status code: 400, request id: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

Expected Behavior

No errors when applying changes.

Potential solution

If this is not possible we can work around this by making the description of the parameter group configurable, so that the Managed by Terraform value can be used for existing resources.

syphernl commented 2 years ago

/cc cc'ing in @nitrocode as he is the author of the changes done in v0.41.3.

syphernl commented 2 years ago

I have created two PR's with slightly different directions:

nitrocode commented 2 years ago

When I looked over the terraform resource in their code, I didn't see a ForceNew in the description so figured it was fine to add the descriptions. Good catch.

Couldn't we simply ignore changes if the description is changed ?

  lifecycle {
    ignore_changes = [
      description,
    ]
  }
syphernl commented 2 years ago

@nitrocode Oh wow, that actually sounds like a far easier solution 😄

The only downside is that the description then can not be changed at all (which makes #143 not longer functional). But I don't think anyone would actually need to do so since AWS requires a new PG to be created instead of being able to update it in-place.

If we go that route we can probably better close #143. PR #144 can be rewritten to go this route instead (or replaced, whichever is most practical).

nitrocode commented 2 years ago

I added the lifecycle ignore to 143 and the updated description is opt in if you set the description input to null.

I also set the last release as a pre-release

nitrocode commented 2 years ago

@syphernl thanks again for notifying us so quickly!