From 9eca996e216f81958265a5849fde22db704f21fb Mon Sep 17 00:00:00 2001 From: ysyneu Date: Wed, 12 Aug 2026 02:41:05 -0700 Subject: [PATCH 1/2] fix: stop coercing server-side false/0 to Null on read types.Bool and types.Int64 in terraform-plugin-framework are Null by default, so `if cond { result.X = types.BoolValue(true) }` (no else) silently leaves the attribute Null whenever the server's value is the type's zero value. A config that explicitly sets the attribute to false/0 then diffs against a Null state on every subsequent plan - drift that never converges. Fixed the read/mapping helpers for attributes where zero is a real, documented, distinct value: - flashduty_channel: group.all_equals_required, group.time_window ("0 means merge until incident closes") - flashduty_route: cases[].fallthrough - flashduty_schedule: layers[].fair_rotation, layers[].handoff_time, layers[].restrict_mode ("0 = none"), layers[].mask_continuous_enabled, notify.advance_in_time Each of these was Optional-only (not Computed) in its schema, so simply removing the `if` and always writing a concrete value would trade the drift for a stricter failure: "Provider produced inconsistent result after apply" whenever a config omits the attribute and the provider now writes a concrete zero into it. Instead each attribute schema is changed to Optional+Computed with a static default of false/0, following the pattern already used elsewhere in this provider (e.g. channel.is_private), and the mapping helper now writes the value unconditionally. group.i_score_threshold and layers[].layer_end keep their original guarded behavior: i_score_threshold's documented valid range is 0.5-1.0, so 0 is never a legal configured value; layer_end is a Unix timestamp where 0 has no meaning as a real end time distinct from "no end configured", so collapsing it to Null loses nothing a user could have intentionally set. flashduty_schedule's readNotify() also collapses to a nil notify block when the server returns an all-zero-value struct for a resource that was never configured with `notify` (avoiding drift the other direction). That check previously read the mapped model's AdvanceInTime.IsNull(), which always becomes false now that AdvanceInTime is written unconditionally; it's rewritten to check the raw response struct instead, so the empty-notify case still collapses correctly. Added unit tests for the affected mapping helpers asserting that a server-side false/0 produces a concrete BoolValue(false)/Int64Value(0) rather than Null, and that a fully empty schedule notify still collapses to nil. --- .../provider/flashduty_channel_resource.go | 16 +- .../flashduty_read_path_zero_value_test.go | 189 ++++++++++++++++++ internal/provider/flashduty_route_resource.go | 7 +- .../provider/flashduty_schedule_resource.go | 47 +++-- 4 files changed, 231 insertions(+), 28 deletions(-) create mode 100644 internal/provider/flashduty_read_path_zero_value_test.go diff --git a/internal/provider/flashduty_channel_resource.go b/internal/provider/flashduty_channel_resource.go index eb05348..fcfb49d 100644 --- a/internal/provider/flashduty_channel_resource.go +++ b/internal/provider/flashduty_channel_resource.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/resource" "github.com/hashicorp/terraform-plugin-framework/resource/schema" "github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/int64default" "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier" "github.com/hashicorp/terraform-plugin-framework/types" @@ -143,6 +144,8 @@ func (r *ChannelResource) Schema(ctx context.Context, req resource.SchemaRequest }, "time_window": schema.Int64Attribute{ Optional: true, + Computed: true, + Default: int64default.StaticInt64(0), MarkdownDescription: "Time window in minutes (0-60). 0 means merge until incident closes.", }, "cases": schema.ListNestedAttribute{ @@ -186,6 +189,8 @@ func (r *ChannelResource) Schema(ctx context.Context, req resource.SchemaRequest }, "all_equals_required": schema.BoolAttribute{ Optional: true, + Computed: true, + Default: booldefault.StaticBool(false), MarkdownDescription: "Whether all grouping dimensions must be present. Default `false`.", }, "i_keys": schema.ListAttribute{ @@ -512,12 +517,11 @@ func (r *ChannelResource) readGroup(ctx context.Context, g *client.ChannelGroup, Method: types.StringValue(g.Method), } - if g.TimeWindow != 0 { - result.TimeWindow = types.Int64Value(int64(g.TimeWindow)) - } - if g.AllEqualsRequired { - result.AllEqualsRequired = types.BoolValue(true) - } + result.TimeWindow = types.Int64Value(int64(g.TimeWindow)) + result.AllEqualsRequired = types.BoolValue(g.AllEqualsRequired) + // i_score_threshold has a documented valid range of 0.5-1.0, so 0 is + // never a legal, distinct value the server would return for a + // configured group - it signals "not applicable" and stays Null. if g.IScoreThreshold != 0 { result.IScoreThreshold = types.Float64Value(g.IScoreThreshold) } diff --git a/internal/provider/flashduty_read_path_zero_value_test.go b/internal/provider/flashduty_read_path_zero_value_test.go new file mode 100644 index 0000000..f13bd81 --- /dev/null +++ b/internal/provider/flashduty_read_path_zero_value_test.go @@ -0,0 +1,189 @@ +package provider + +import ( + "context" + "testing" + + "terraform-provider-flashduty/internal/client" + + "github.com/hashicorp/terraform-plugin-framework/diag" +) + +// These tests guard against a class of bug where a server-side false/0 is +// mapped to a Null attribute value instead of a concrete BoolValue(false) / +// Int64Value(0). In terraform-plugin-framework, Null is the zero value of +// types.Bool / types.Int64, so an `if cond { set value }` with no else +// silently drops the case where the server's value is the type's zero +// value, producing permanent drift between state and a config that +// explicitly set the attribute to its zero value. + +func TestChannelReadGroupZeroValues(t *testing.T) { + r := &ChannelResource{} + var diags diag.Diagnostics + + g := &client.ChannelGroup{ + Method: "p", + AllEqualsRequired: false, + TimeWindow: 0, + IScoreThreshold: 0, + } + + result := r.readGroup(context.Background(), g, &diags) + if diags.HasError() { + t.Fatalf("unexpected diagnostics: %v", diags) + } + + if result.AllEqualsRequired.IsNull() { + t.Error("AllEqualsRequired: got Null, want BoolValue(false)") + } else if result.AllEqualsRequired.ValueBool() != false { + t.Errorf("AllEqualsRequired: got %v, want false", result.AllEqualsRequired.ValueBool()) + } + + if result.TimeWindow.IsNull() { + t.Error("TimeWindow: got Null, want Int64Value(0)") + } else if result.TimeWindow.ValueInt64() != 0 { + t.Errorf("TimeWindow: got %v, want 0", result.TimeWindow.ValueInt64()) + } + + // IScoreThreshold has a documented valid range of 0.5-1.0, so 0 is not + // a legal, distinct value the server would return for a configured + // group - it stays Null on zero (not fixed). + if !result.IScoreThreshold.IsNull() { + t.Errorf("IScoreThreshold: got %v, want Null (0 is out of the documented 0.5-1.0 range)", result.IScoreThreshold.ValueFloat64()) + } +} + +func TestRouteMapRouteToModelZeroValues(t *testing.T) { + r := &RouteResource{} + var diags diag.Diagnostics + + route := &client.Route{ + Cases: []client.RouteCase{ + { + If: []client.RouteFilter{{Key: "title", Oper: "IN", Vals: []string{"x"}}}, + Fallthrough: false, + }, + }, + } + + model := &RouteResourceModel{} + r.mapRouteToModel(context.Background(), route, model, &diags) + if diags.HasError() { + t.Fatalf("unexpected diagnostics: %v", diags) + } + + got := model.Cases[0].Fallthrough + if got.IsNull() { + t.Error("Fallthrough: got Null, want BoolValue(false)") + } else if got.ValueBool() != false { + t.Errorf("Fallthrough: got %v, want false", got.ValueBool()) + } +} + +func TestScheduleReadLayersZeroValues(t *testing.T) { + r := &ScheduleResource{} + var diags diag.Diagnostics + + layers := []client.ScheduleLayer{ + { + LayerName: "L1", + Mode: 0, + LayerStart: 1704038400, + RotationUnit: "day", + RotationValue: 1, + FairRotation: false, + HandoffTime: 0, + RestrictMode: 0, + MaskContinuousEnabled: false, + LayerEnd: 0, + }, + } + + result := r.readLayers(context.Background(), layers, &diags) + if diags.HasError() { + t.Fatalf("unexpected diagnostics: %v", diags) + } + if len(result) != 1 { + t.Fatalf("got %d layers, want 1", len(result)) + } + l := result[0] + + if l.FairRotation.IsNull() { + t.Error("FairRotation: got Null, want BoolValue(false)") + } else if l.FairRotation.ValueBool() != false { + t.Errorf("FairRotation: got %v, want false", l.FairRotation.ValueBool()) + } + + if l.HandoffTime.IsNull() { + t.Error("HandoffTime: got Null, want Int64Value(0)") + } else if l.HandoffTime.ValueInt64() != 0 { + t.Errorf("HandoffTime: got %v, want 0", l.HandoffTime.ValueInt64()) + } + + if l.RestrictMode.IsNull() { + t.Error("RestrictMode: got Null, want Int64Value(0)") + } else if l.RestrictMode.ValueInt64() != 0 { + t.Errorf("RestrictMode: got %v, want 0", l.RestrictMode.ValueInt64()) + } + + if l.MaskContinuousEnabled.IsNull() { + t.Error("MaskContinuousEnabled: got Null, want BoolValue(false)") + } else if l.MaskContinuousEnabled.ValueBool() != false { + t.Errorf("MaskContinuousEnabled: got %v, want false", l.MaskContinuousEnabled.ValueBool()) + } + + // LayerEnd is a Unix timestamp, not a duration. 0 has no meaning as a + // real end time distinct from "no end configured", so it stays Null + // on zero (not fixed). + if !l.LayerEnd.IsNull() { + t.Errorf("LayerEnd: got %v, want Null (0 is not a legal end timestamp)", l.LayerEnd.ValueInt64()) + } +} + +func TestScheduleReadNotifyZeroAdvanceInTime(t *testing.T) { + r := &ScheduleResource{} + var diags diag.Diagnostics + + // A real notify block, explicitly configured with advance_in_time = 0, + // but with other settings present so the server would never omit it. + notify := &client.ScheduleNotify{ + AdvanceInTime: 0, + By: &client.NotifyBy{ + FollowPreference: true, + }, + } + + result := r.readNotify(context.Background(), notify, &diags) + if diags.HasError() { + t.Fatalf("unexpected diagnostics: %v", diags) + } + if result == nil { + t.Fatal("got nil ScheduleNotifyModel, want non-nil (By is set)") + } + + if result.AdvanceInTime.IsNull() { + t.Error("AdvanceInTime: got Null, want Int64Value(0)") + } else if result.AdvanceInTime.ValueInt64() != 0 { + t.Errorf("AdvanceInTime: got %v, want 0", result.AdvanceInTime.ValueInt64()) + } +} + +// A genuinely empty notify block (server returns a non-nil struct with +// every field at its zero value, because the resource was never +// configured with `notify`) must still collapse to nil - it must not be +// resurrected as a non-nil block full of zero values just because +// AdvanceInTime is no longer Null-on-zero. +func TestScheduleReadNotifyFullyEmptyCollapsesToNil(t *testing.T) { + r := &ScheduleResource{} + var diags diag.Diagnostics + + notify := &client.ScheduleNotify{} + + result := r.readNotify(context.Background(), notify, &diags) + if diags.HasError() { + t.Fatalf("unexpected diagnostics: %v", diags) + } + if result != nil { + t.Errorf("got %+v, want nil for a fully empty notify block", result) + } +} diff --git a/internal/provider/flashduty_route_resource.go b/internal/provider/flashduty_route_resource.go index 9c20dcd..0fa736b 100644 --- a/internal/provider/flashduty_route_resource.go +++ b/internal/provider/flashduty_route_resource.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/diag" "github.com/hashicorp/terraform-plugin-framework/resource" "github.com/hashicorp/terraform-plugin-framework/resource/schema" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault" "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier" "github.com/hashicorp/terraform-plugin-framework/schema/validator" @@ -121,6 +122,8 @@ func (r *RouteResource) Schema(ctx context.Context, req resource.SchemaRequest, "fallthrough": schema.BoolAttribute{ MarkdownDescription: "Whether to continue matching after this rule.", Optional: true, + Computed: true, + Default: booldefault.StaticBool(false), }, "routing_mode": schema.StringAttribute{ MarkdownDescription: "Routing mode (standard, name_mapping).", @@ -368,9 +371,7 @@ func (r *RouteResource) mapRouteToModel(ctx context.Context, route *client.Route for i, routeCase := range route.Cases { caseModel := RouteCaseModel{} - if routeCase.Fallthrough { - caseModel.Fallthrough = types.BoolValue(true) - } + caseModel.Fallthrough = types.BoolValue(routeCase.Fallthrough) if routeCase.RoutingMode != "" { caseModel.RoutingMode = types.StringValue(routeCase.RoutingMode) } diff --git a/internal/provider/flashduty_schedule_resource.go b/internal/provider/flashduty_schedule_resource.go index d8ef90d..5356835 100644 --- a/internal/provider/flashduty_schedule_resource.go +++ b/internal/provider/flashduty_schedule_resource.go @@ -13,6 +13,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/path" "github.com/hashicorp/terraform-plugin-framework/resource" "github.com/hashicorp/terraform-plugin-framework/resource/schema" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault" "github.com/hashicorp/terraform-plugin-framework/resource/schema/int64default" "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier" @@ -164,14 +165,20 @@ func (r *ScheduleResource) Schema(ctx context.Context, req resource.SchemaReques }, "fair_rotation": schema.BoolAttribute{ Optional: true, + Computed: true, + Default: booldefault.StaticBool(false), MarkdownDescription: "Whether to enable fair rotation.", }, "handoff_time": schema.Int64Attribute{ Optional: true, + Computed: true, + Default: int64default.StaticInt64(0), MarkdownDescription: "The handoff time in seconds.", }, "restrict_mode": schema.Int64Attribute{ Optional: true, + Computed: true, + Default: int64default.StaticInt64(0), MarkdownDescription: "The restriction mode. 0 = none.", }, "restrict_periods": schema.ListNestedAttribute{ @@ -203,6 +210,8 @@ func (r *ScheduleResource) Schema(ctx context.Context, req resource.SchemaReques }, "mask_continuous_enabled": schema.BoolAttribute{ Optional: true, + Computed: true, + Default: booldefault.StaticBool(false), MarkdownDescription: "Whether continuous mask is enabled.", }, "groups": schema.ListNestedAttribute{ @@ -245,6 +254,8 @@ func (r *ScheduleResource) Schema(ctx context.Context, req resource.SchemaReques Attributes: map[string]schema.Attribute{ "advance_in_time": schema.Int64Attribute{ Optional: true, + Computed: true, + Default: int64default.StaticInt64(0), MarkdownDescription: "Advance notification time in seconds.", }, "fixed_time": schema.SingleNestedAttribute{ @@ -539,21 +550,16 @@ func (r *ScheduleResource) readLayers(_ context.Context, layers []client.Schedul RotationValue: types.Int64Value(int64(layer.RotationValue)), } + // layer_end is a Unix timestamp, not a duration: 0 has no meaning + // as a real end time distinct from "no end configured", so it + // stays Null on zero. if layer.LayerEnd != 0 { l.LayerEnd = types.Int64Value(layer.LayerEnd) } - if layer.FairRotation { - l.FairRotation = types.BoolValue(true) - } - if layer.HandoffTime != 0 { - l.HandoffTime = types.Int64Value(int64(layer.HandoffTime)) - } - if layer.RestrictMode != 0 { - l.RestrictMode = types.Int64Value(int64(layer.RestrictMode)) - } - if layer.MaskContinuousEnabled { - l.MaskContinuousEnabled = types.BoolValue(true) - } + l.FairRotation = types.BoolValue(layer.FairRotation) + l.HandoffTime = types.Int64Value(int64(layer.HandoffTime)) + l.RestrictMode = types.Int64Value(int64(layer.RestrictMode)) + l.MaskContinuousEnabled = types.BoolValue(layer.MaskContinuousEnabled) for _, rp := range layer.RestrictPeriods { l.RestrictPeriods = append(l.RestrictPeriods, ScheduleRestrictPeriodModel{ @@ -655,10 +661,17 @@ func (r *ScheduleResource) readNotify(_ context.Context, notify *client.Schedule return nil } - result := &ScheduleNotifyModel{} + // The server returns a non-nil ScheduleNotify struct even when the + // resource was never configured with a `notify` block. Detect that + // case from the raw response (not the mapped model, since + // AdvanceInTime is always given a concrete value below) and collapse + // it to nil so an unconfigured `notify` doesn't reappear in state. + if notify.AdvanceInTime == 0 && notify.FixedTime == nil && notify.By == nil && len(notify.Webhooks) == 0 { + return nil + } - if notify.AdvanceInTime != 0 { - result.AdvanceInTime = types.Int64Value(int64(notify.AdvanceInTime)) + result := &ScheduleNotifyModel{ + AdvanceInTime: types.Int64Value(int64(notify.AdvanceInTime)), } if notify.FixedTime != nil { @@ -698,9 +711,5 @@ func (r *ScheduleResource) readNotify(_ context.Context, notify *client.Schedule result.Webhooks = append(result.Webhooks, whModel) } - if result.AdvanceInTime.IsNull() && result.FixedTime == nil && result.By == nil && len(result.Webhooks) == 0 { - return nil - } - return result } From ac3c84a031e1ffb9c85a4edda1423bf4b2093cee Mon Sep 17 00:00:00 2001 From: ysyneu Date: Wed, 12 Aug 2026 02:49:44 -0700 Subject: [PATCH 2/2] docs: regenerate escalate_rule reference to match the schema default --- docs/resources/escalate_rule.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/resources/escalate_rule.md b/docs/resources/escalate_rule.md index 5821164..cf6a945 100644 --- a/docs/resources/escalate_rule.md +++ b/docs/resources/escalate_rule.md @@ -119,7 +119,7 @@ resource "flashduty_escalate_rule" "critical" { - `aggr_window` (Number) Aggregation window in seconds (0-3600). - `description` (String) The description of the rule. - `filters` (Attributes List) Alert matching filter conditions (OR between groups, AND within conditions). (see [below for nested schema](#nestedatt--filters)) -- `priority` (Number) The priority of the escalation rule for ordering. +- `priority` (Number) The priority of the escalation rule for ordering. Defaults to `1`. - `time_filters` (Attributes List) Time-based filter conditions for when this rule applies. (see [below for nested schema](#nestedatt--time_filters)) ### Read-Only