Skip to content

Conversation

@bai
Copy link

@bai bai commented Jul 7, 2025

When creating a new valkey cluster with gcs_source, we are instructed to pass a comma-separated list. However, it's not accepted by the underlying terraform resource. This PR fixes that mismatch 🙏

@bai bai requested review from a team, ayushmjain, imrannayer and q2w as code owners July 7, 2025 09:42
@bai bai force-pushed the fix-gcs-source branch from 570a7f9 to 35ba636 Compare July 7, 2025 11:13
Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @bai

for_each = var.gcs_source != null ? ["gcs_source"] : []
content {
uris = var.gcs_source
uris = split(",", var.gcs_source)
Copy link
Member

Choose a reason for hiding this comment

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

If a list is expected as input, it would be better to change gcs_source to a list type.

Copy link
Author

Choose a reason for hiding this comment

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

Can do, but as a note - that goes kinda against the way lists are handled elsewhere in google terraform modules, for example node_locations comma separated list in https://github.com/terraform-google-modules/terraform-google-kubernetes-engine (right in the root readme).

I actually do prefer using a proper list by the way, just wanted to double check with you.

Copy link
Member

Choose a reason for hiding this comment

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

It escapes me why we did it that way for node_locations (maybe because node_pools were untyped) but I am leaning towards list for this case. @imrannayer @q2w any thoughts?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants