Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new platform field to the Redis database resource while deprecating the existing region field. The change allows users to specify the cloud platform (AWS or GCP) for global databases instead of using specific region strings.
Changes:
- Added
platformfield toCreateDatabaseRequeststruct with omitempty JSON tag - Modified resource schema to add
platformas optional/computed field and deprecateregionfield - Implemented platform inference logic in read operation based on region values ("gcp-global" → "gcp", "global" → "aws")
- Updated .gitignore to exclude .env and dev.tfrc files
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| upstash/redis/database/types.go | Added Platform field to CreateDatabaseRequest struct with omitempty tag |
| upstash/redis/database/resource.go | Added platform schema field definition, deprecated region field, added AtLeastOneOf validation |
| upstash/redis/database/crud.go | Added platform inference logic in resourceDatabaseRead and platform field passing in resourceDatabaseCreate |
| .gitignore | Added exclusions for .env and dev.tfrc development files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Type: schema.TypeString, | ||
| Optional: true, | ||
| ForceNew: true, | ||
| Computed: true, | ||
| Deprecated: "The 'region' field is deprecated. Use 'platform' field instead.", |
There was a problem hiding this comment.
The deprecation message for the region field is misleading. It states "Use 'platform' field instead", but the platform field serves a different purpose - it specifies whether to use AWS or GCP, not the specific region. The region field specified actual regions like "us-east-1", "global", "gcp-global", etc. Users migrating from region to platform will lose the ability to specify a specific region, which appears to be a breaking change. The deprecation message should clarify the relationship between these fields and explain the migration path more clearly.
| Type: schema.TypeString, | |
| Optional: true, | |
| ForceNew: true, | |
| Computed: true, | |
| Deprecated: "The 'region' field is deprecated. Use 'platform' field instead.", | |
| Type: schema.TypeString, | |
| Optional: true, | |
| ForceNew: true, | |
| Computed: true, | |
| Deprecated: "The 'region' field is deprecated and no longer used to select a specific region. Use the 'platform' field to choose the cloud provider (aws or gcp); the actual region is now determined automatically by Upstash and cannot be configured via this field.", |
| "platform": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Computed: true, | ||
| ForceNew: true, | ||
| Description: "Platform of the database. Can be one of [aws, gcp]", | ||
| AtLeastOneOf: []string{"platform", "region"}, | ||
| }, |
There was a problem hiding this comment.
The new platform field and the region deprecation should be covered by integration tests. The existing test in integrationtesting/upstash_redis_database_test.go uses the region field, but there are no tests for the new platform field. Consider adding test cases that use the platform field to ensure the new functionality works correctly and that backward compatibility with the region field is maintained.
upstash/redis/database/resource.go
Outdated
| Optional: true, | ||
| Computed: true, | ||
| ForceNew: true, | ||
| Description: "Platform of the database. Can be one of [aws, gcp]", |
There was a problem hiding this comment.
The description states the platform can be "one of [aws, gcp]" but uses non-standard documentation format. For consistency with Terraform schema conventions, consider using a format like "Possible values: aws, gcp" or listing them on separate lines. This makes the documentation clearer and more consistent with typical Terraform provider documentation patterns.
| Description: "Platform of the database. Can be one of [aws, gcp]", | |
| Description: "Platform of the database. Possible values: `aws`, `gcp`.", |
upstash/redis/database/crud.go
Outdated
| var platform string | ||
| switch database.Region { | ||
| case "gcp-global": | ||
| platform = "gcp" | ||
| case "global": | ||
| platform = "aws" | ||
| } |
There was a problem hiding this comment.
The platform inference logic only handles "gcp-global" and "global" cases. When database.Region is neither of these (e.g., specific regions like "us-east-1", "eu-west-1"), the platform variable will be an empty string. This will set the platform field to an empty string in the state, which could break existing configurations or cause confusion for users with regional databases. The logic should handle all region cases to properly infer the platform.
upstash/redis/database/resource.go
Outdated
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Computed: true, | ||
| ForceNew: true, | ||
| Description: "Platform of the database. Can be one of [aws, gcp]", | ||
| AtLeastOneOf: []string{"platform", "region"}, |
There was a problem hiding this comment.
The AtLeastOneOf constraint requires that either "platform" or "region" must be provided. However, the "region" field is marked as deprecated. This creates a confusing situation where users must still provide a deprecated field if they don't provide the platform field. A better approach would be to make platform Required (not Optional) and remove the AtLeastOneOf constraint, or ensure proper validation that guides users to use platform instead of region for new resources.
| Type: schema.TypeString, | |
| Optional: true, | |
| Computed: true, | |
| ForceNew: true, | |
| Description: "Platform of the database. Can be one of [aws, gcp]", | |
| AtLeastOneOf: []string{"platform", "region"}, | |
| Type: schema.TypeString, | |
| Required: true, | |
| ForceNew: true, | |
| Description: "Platform of the database. Can be one of [aws, gcp]", |
There was a problem hiding this comment.
yes. No need to use AtLeastOneOf. marking the other as deprecated should be enough
| "platform": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Computed: true, | ||
| ForceNew: true, | ||
| Description: "Platform of the database. Can be one of [aws, gcp]", | ||
| AtLeastOneOf: []string{"platform", "region"}, | ||
| }, |
There was a problem hiding this comment.
The platform field lacks input validation. According to the description, it should be one of "aws" or "gcp", but there's no ValidateFunc or ValidateDiagFunc to enforce this. Users could provide invalid values like "azure" or "aws-east" which would likely cause API errors. Add validation to ensure only valid platform values are accepted.
| Type: schema.TypeString, | ||
| Optional: true, | ||
| ForceNew: true, | ||
| Computed: true, | ||
| Deprecated: "The 'region' field is deprecated. Use 'platform' field instead.", |
There was a problem hiding this comment.
When both "region" and "platform" are provided by the user, there's no validation to ensure they are compatible. For example, a user could specify platform="gcp" with region="us-east-1" (an AWS region), or platform="aws" with region="gcp-global". This could lead to API errors or unexpected behavior. Consider adding validation to ensure the platform and region values are consistent when both are provided.
| Type: schema.TypeString, | |
| Optional: true, | |
| ForceNew: true, | |
| Computed: true, | |
| Deprecated: "The 'region' field is deprecated. Use 'platform' field instead.", | |
| Type: schema.TypeString, | |
| Optional: true, | |
| ForceNew: true, | |
| Computed: true, | |
| Deprecated: "The 'region' field is deprecated. Use 'platform' field instead.", | |
| ConflictsWith: []string{"platform"}, |
upstash/redis/database/resource.go
Outdated
| "platform": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| Computed: true, |
There was a problem hiding this comment.
it is not computed, given by user input
There was a problem hiding this comment.
it was specifically set to be computed since, if, a user wants to use the new field instead of region, now platform field is changed from empty to some value like aws, since it has ForceNew field, terraform will destroy the existing and create a new one even the existing one was already in aws.
as a workaround, i will write a custom diff checker for terraform, it will check if the platform actually changed and then make a plan. but if it seems too complex for this, we can go back to computed: true
burak-upstash
left a comment
There was a problem hiding this comment.
We need to update the docs related to the change. Please check readme to see how it can be done
burak-upstash
left a comment
There was a problem hiding this comment.
Additional note (not in diff): resource.go:100 and resource.go:108 — the primary_region and read_regions descriptions still reference region='global', but region is now deprecated. Users following the new platform field guidance won't know these fields apply to them.
Consider updating to something like:
"Primary region for the database (Only works if platform is set or region='global'. Can be one of [...])"
| if _, ok := data.GetOk("platform"); ok { | ||
| var platform string | ||
| switch database.Region { | ||
| case "gcp-global": | ||
| platform = "gcp" | ||
| case "global": | ||
| platform = "aws" | ||
| } | ||
| mapping["platform"] = platform | ||
| } |
There was a problem hiding this comment.
Bug: When data.GetOk("platform") is true but the database region is not "global" or "gcp-global" (e.g. a legacy single-region DB with region "us-east-1"), the switch has no matching case and platform stays "". This empty string gets written to state via mapping["platform"] = platform, causing a perpetual plan diff on every run (config says "aws", state says "").
Fix — only set platform in the mapping when the inferred value is non-empty:
if _, ok := data.GetOk("platform"); ok {
var platform string
switch database.Region {
case "gcp-global":
platform = "gcp"
case "global":
platform = "aws"
}
if platform != "" {
mapping["platform"] = platform
}
}| region := data.Get("region").(string) | ||
| var platform string | ||
| switch region { | ||
| case "gcp-global": | ||
| platform = "gcp" | ||
| case "global": | ||
| platform = "aws" | ||
| } |
There was a problem hiding this comment.
Minor: this region-to-platform switch logic is duplicated in crud.go:136-142. Consider extracting a shared helper so both stay in sync if the mapping ever changes (e.g. a new platform is added).
No description provided.