feat(TPG >=5.24)!: Allow modifying of node type in redis cluster sub-module

[ivankennethwang]

fix #205

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[ivankennethwang]

@imrannayer would this be ok?

[edubxb]

Awesome, thanks for this contribution @ivankennethwang. We also need it!

[ivankennethwang]

Welcome. Let me know if there is anything else I can do. Happy to help!

[imrannayer]

Welcome. Let me know if there is anything else I can do. Happy to help!

@ivankennethwang Can you look at my comment and add appropriate output.

[ivankennethwang]

Welcome. Let me know if there is anything else I can do. Happy to help!

@ivankennethwang Can you look at my comment and add appropriate output.

Thanks @imrannayer. Where can I see your comments? I don't see them here.

[imrannayer]

@ivankennethwang

https://github.com/terraform-google-modules/terraform-google-memorystore/pull/206/files/2960c746088c47e9df09374f6d95b1fbf2c699c1

[ivankennethwang]

@imrannayer i sincerely apologize but i really don't see any github comments. :(

[imrannayer]

@ivankennethwang just scroll up on this screen and you will see code change request.

[ivankennethwang]

@imrannayer sorry this is what i see, i really dont see any code change requests, or any other comment in the files changed tab.

[imrannayer]

@ivankennethwang I am not sure whayt interface you are using but here is whats neede.

**@ivankennethwang you need to add node_type in output here. Add following lines

output "node_type" {
  description = "The redis cluster node type"
  value       = module.redis_cluster.redis_cluster.node_type
}
```**

[ivankennethwang]

@ivankennethwang I am not sure whayt interface you are using but here is whats neede.

**@ivankennethwang you need to add node_type in output here. Add following lines

output "node_type" {
  description = "The redis cluster node type"
  value       = module.redis_cluster.redis_cluster.node_type
}
```**

I'm just using github.com in Chrome browser. 😅

Btw, updated and thanks! 🙇‍♂️

[imrannayer]

@ivankennethwang lint test is failing. Can you plz execute

make docker_generate_docs

[ivankennethwang]

@ivankennethwang lint test is failing. Can you plz execute

make docker_generate_docs

done, thanks!

[imrannayer]

/gcbrun

[imrannayer]

• @ivankennethwang default value should be null. Provider automatically picks default value as REDIS_HIGHMEM_MEDIUM.

• Can u also update version.tf for redis_cluster sub-module. Set min provider version to 5.24 as it was part of 5.24 release.

• Add an upgrade to v9 doc in docs folder mentioning that this will be a breaking change due to change in min provider version. Here is an example

[ivankennethwang]

• @ivankennethwang default value should be null. Provider automatically picks default value as REDIS_HIGHMEM_MEDIUM.
• Can u also update version.tf for redis_cluster sub-module. Set min provider version to 5.24 as it was part of 5.24 release.
• Add an upgrade to v9 doc in docs folder mentioning that this will be a breaking change due to change in min provider version. Here is an example

Sorry, this is my first time contributing ever in a mature repository.

May I know when do we update the README.md, CHANGELOG.md and versions.tf at root level? I suppose those should be changed to include 5.24 and v9.0 also?

btw, updated.

[imrannayer]

• @ivankennethwang default value should be null. Provider automatically picks default value as REDIS_HIGHMEM_MEDIUM.
• Can u also update version.tf for redis_cluster sub-module. Set min provider version to 5.24 as it was part of 5.24 release.
• Add an upgrade to v9 doc in docs folder mentioning that this will be a breaking change due to change in min provider version. Here is an example

Sorry, this is my first time contributing ever in a mature repository.

May I know when do we update the README.md, CHANGELOG.md and versions.tf at root level? I suppose those should be changed to include 5.24 and v9.0 also?

btw, updated.

CHANGELOG will be updated automatically. README and versions.tf will be at module level.

[imrannayer]

@ivankennethwang you are missing default = null

variable "node_type" {
  description = "The nodeType for the Redis cluster. If not provided, REDIS_HIGHMEM_MEDIUM will be used as default Possible values are: REDIS_SHARED_CORE_NANO, REDIS_HIGHMEM_MEDIUM, REDIS_HIGHMEM_XLARGE, REDIS_STANDARD_SMALL."
  type        = string
  default     = null
}

[ivankennethwang]

@ivankennethwang you are missing default = null

variable "node_type" {
  description = "The nodeType for the Redis cluster. If not provided, REDIS_HIGHMEM_MEDIUM will be used as default Possible values are: REDIS_SHARED_CORE_NANO, REDIS_HIGHMEM_MEDIUM, REDIS_HIGHMEM_XLARGE, REDIS_STANDARD_SMALL."
  type        = string
  default     = null
}

updated, sorry about that.

[ivankennethwang]

how come for authorization_mode we set default = "AUTH_MODE_DISABLED" though?

i thought node_type is quite similar to this[1], should we set it to null too?

[1] https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/redis_cluster#authorization_mode

[imrannayer]

@ivankennethwang lint failed on document generation check

[imrannayer]

how come for authorization_mode we set default = "AUTH_MODE_DISABLED" though?

i thought node_type is quite similar to this[1], should we set it to null too?

[1] https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/redis_cluster#authorization_mode

Could have been set to null.

[ivankennethwang]

how come for authorization_mode we set default = "AUTH_MODE_DISABLED" though?
i thought node_type is quite similar to this[1], should we set it to null too?
[1] https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/redis_cluster#authorization_mode

Could have been set to null.

Perhaps we can put it in a different pull request.

@ivankennethwang lint failed on document generation check

Updated, thanks!

[imrannayer]

/gcbrun