feat: [Redis] add support for data tiering nodes #783
Conversation
aws-redis-cluster.yml
Outdated
| For more information about Node Types and supported Redis versions see | ||
| https://docs.aws.amazon.com/AmazonElastiCache/latest/red-ug/CacheNodes.SupportedTypes.html | ||
| default: false | ||
| prohibit_update: true |
There was a problem hiding this comment.
Prohibiting updating this field introduces unexpected behaviour:
- node_type field is updatable
- if node_type is set to an
r6gdnode_type but is later changed to a nonr6gdnode_type,
data_tiering_enabled either (I don't know which will happen):- causes the node_type change to fail
- node_type change succeeds but
data_tiering_enabled: trueno longer represents the reality
There was a problem hiding this comment.
@fnaranjo-vmw The node_type change wouldn't succeed as you cannot move from a data_tiering enabled node type to a non-data-tiering node (and vice versa). So I think prohibiting the update in this field actually has the effect of providing rapid feedback to the user.
There was a problem hiding this comment.
Perfect. Everything looks nice then!
Great job @zucchinidev !
There was a problem hiding this comment.
Hey @pivotal-marcela-campo , do you know if we could encode this logic (changing from data-tiering to non-data tiering and vice-versa) as a terraform tests?
Or this logic only happens in the IAAS, and as such can't be expressed as a terraform test?
| redisServiceProviderDisplayName = "VMware" | ||
| redisCustomPlanName = "custom-sample" | ||
| redisCustomPlanID = "c7f64994-a1d9-4e1f-9491-9d8e56bbf146" | ||
| deprecatedCacheSizePlanName = "deprecated-cachesize-sample" |
There was a problem hiding this comment.
nitpick: Maybe by introducing a new line between redisCustomPlanID and deprecatedCacheSizePlanName go formatter wouldn't try to reformat untouched lines to preserve alignment.
Having git/Github diffs only highlight added/changed lines is a big help when reviewing the code.
However, feel free to leave it as it is. But maybe keep it in mind for future commits.
There was a problem hiding this comment.
Also, I see you have done some cleanup of the code from cache_size.
I would recommend adding these changes in a different PR to keep this PR focused on the changes needed to make data_tiering work.
Also, feel free to be more strict when reviewing my PRs.
I wouldn't be bothered by you asking me to modify my PR to use less hardcoded strings in tests.
There was a problem hiding this comment.
I believe this PR is small enough that it is ok to leave the cleanup changes in it.
There was a problem hiding this comment.
aws-redis-cluster.yml
Outdated
| For more information about Node Types and supported Redis versions see | ||
| https://docs.aws.amazon.com/AmazonElastiCache/latest/red-ug/CacheNodes.SupportedTypes.html | ||
| default: false | ||
| prohibit_update: true |
There was a problem hiding this comment.
@fnaranjo-vmw The node_type change wouldn't succeed as you cannot move from a data_tiering enabled node type to a non-data-tiering node (and vice versa). So I think prohibiting the update in this field actually has the effect of providing rapid feedback to the user.
#184160811
Checklist: