[FUNK-2191] Expose disconnect_all_warehouses argument#221
[FUNK-2191] Expose disconnect_all_warehouses argument#221vivekabhiram wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR exposes the disconnect_all_warehouses argument in the Terraform provider for Segment sources, allowing users to control warehouse connections during source creation instead of having it hardcoded to true.
Changes:
- Added
disconnect_all_warehousesas an optional boolean attribute to the source resource schema - Updated the
SourcePlanmodel to include theDisconnectAllWarehousesfield - Modified the
Createmethod to use the user-provided value instead of the hardcodedtruevalue
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/provider/source_resource.go | Added schema definition for disconnect_all_warehouses attribute and updated Create method to use the configured value instead of hardcoded true |
| internal/provider/models/source.go | Added DisconnectAllWarehouses field to SourcePlan struct to capture user configuration |
Comments suppressed due to low confidence (1)
internal/provider/models/source.go:34
- The
SourceStatestruct is missing theDisconnectAllWarehousesfield that was added toSourcePlan. Since this is an optional configuration field that users may want to track in state, it should be included in the state representation as well to maintain consistency between the plan and state models. This ensures that Terraform can properly track and manage this attribute throughout the resource lifecycle (create, read, update).
type SourceState struct {
Enabled types.Bool `tfsdk:"enabled"`
ID types.String `tfsdk:"id"`
Labels []LabelState `tfsdk:"labels"`
Metadata *SourceMetadataState `tfsdk:"metadata"`
Name types.String `tfsdk:"name"`
Slug types.String `tfsdk:"slug"`
WorkspaceID types.String `tfsdk:"workspace_id"`
WriteKeys []types.String `tfsdk:"write_keys"`
Settings jsontypes.Normalized `tfsdk:"settings"`
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "disconnect_all_warehouses": schema.BoolAttribute{ | ||
| Optional: true, | ||
| Description: "Whether to disconnect all Warehouses from the Source.", | ||
| }, |
There was a problem hiding this comment.
Consider adding a default value or Computed attribute to the schema definition. Since this is an optional field that defaults to a specific behavior when not provided (previously hardcoded to true), users might expect consistent behavior. Additionally, if the API has a default value when this parameter is omitted, the schema should reflect that with either a Default plan modifier or mark it as Computed to allow the API's default to be used and tracked in state.
|
|
||
| disconnectAllWarehouses := true | ||
| var disconnectAllWarehouses *bool | ||
| if !plan.DisconnectAllWarehouses.IsNull() && !plan.DisconnectAllWarehouses.IsUnknown() { |
There was a problem hiding this comment.
The logic for handling the disconnect_all_warehouses parameter only extracts a non-null value from the plan. When the field is null or unknown, disconnectAllWarehouses remains nil, which means the API will receive a nil pointer. Verify that the API correctly interprets a nil value - does it use a default behavior, or should an explicit value always be provided? If the API requires a value when omitted or has a specific default, consider using that default explicitly or document the nil behavior clearly.
| if !plan.DisconnectAllWarehouses.IsNull() && !plan.DisconnectAllWarehouses.IsUnknown() { | |
| if plan.DisconnectAllWarehouses.IsNull() || plan.DisconnectAllWarehouses.IsUnknown() { | |
| // Default behavior when the field is not set in the plan: | |
| // do not disconnect any warehouses. | |
| value := false | |
| disconnectAllWarehouses = &value | |
| } else { |
Currently, the
disconnectAllWarehousesflag is hardcoded totruein the source creation logic. This prevents users from controlling warehouse connections via Terraform.This change:
disconnect_all_warehousesas an optional boolean in thesegment_sourceschema.SourcePlanmodel to capture this field.Createto use the user-provided configuration.