-
Notifications
You must be signed in to change notification settings - Fork 6
fix: handle profiles warehouse schemaName #204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Only send schemaName to API if it differs from the remote state This prevents API failures when the schema name already exists in the warehouse. The Segment API fails if we send a schemaName that matches the current configuration, even though it should be a no-op
Hi thanks for contributing! What error are you seeing here exactly? And do you have reproduction steps? I would like to check if this is something we can fix on the backend. |
Hello Dean, Sure. I had a case opened with Segment Support 575344. Anytime the Segment API is called to update a profiles warehouse with the schemaName field, it fails with the below error. Even if the schemaName is the one already set in Segment for the same profiles warehouse (which looks like a no-op to me). Reproduction steps:
Error :
This error appears anytime the profiles warehouse requires an update of any kind. The real use case we had:
Same error as above. The import works fine, but the definition not being exactly the same between manually created Segment resource and how it is defined in terraform code, terraform tries to update it. I have been investigating the behavior of the Segment API using curl, you can PATCH the profiles warehouse but as soon as you send the schemaName field, the API fails. I have been told by the support it is not workable to change the API so I volunteered to create this PR. Hope this helps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug in profiles warehouse schema name handling to prevent API failures when the schema name already exists in the warehouse configuration. The Segment API fails when receiving a schema name that matches the current configuration, even though it should be a no-op.
- Implements conditional schema name sending based on state comparison
- Adds comprehensive test coverage for schema name handling scenarios
- Updates comments to follow proper formatting conventions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
internal/provider/profiles_warehouse_resource.go | Adds determineSchemaNameForUpdate function and modifies update logic to only send schema name when it differs from remote state |
internal/provider/profiles_warehouse_resource_test.go | Adds new test function and unit tests to verify schema name handling behavior, plus minor comment formatting fixes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
name: "plan_null_state_has_value", | ||
planSchemaName: types.StringNull(), | ||
stateSchemaName: types.StringValue("my-schema"), | ||
expectedResult: nil, // null pointer, not the actual value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment 'null pointer, not the actual value' is confusing and incorrect. This should clarify that nil means the schemaName should not be sent to the API, consistent with other test descriptions.
expectedResult: nil, // null pointer, not the actual value | |
expectedResult: nil, // should not send schemaName to API (prevents API failure) |
Copilot uses AI. Check for mistakes.
name: "plan_unknown_state_has_value", | ||
planSchemaName: types.StringUnknown(), | ||
stateSchemaName: types.StringValue("my-schema"), | ||
expectedResult: nil, // unknown becomes null pointer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment 'unknown becomes null pointer' is misleading. This should clarify that nil means the schemaName should not be sent to the API, consistent with the function's behavior and other test descriptions.
expectedResult: nil, // unknown becomes null pointer | |
expectedResult: nil, // should not send schemaName to API (prevents API failure) |
Copilot uses AI. Check for mistakes.
Going to add a few changes after testing. |
I'm unable to reproduce the issue on my end so far. I set up a Postgres profiles warehouse and set the |
Only send schemaName to API if it differs from the remote state
This prevents API failures when the schema name already exists in the warehouse. The Segment API fails if we send a schemaName that matches the current configuration, even though it should be a no-op