Repository: honeycombio/terraform-provider-honeycombio (language: python)

Issue:
migrate honeycombio_trigger resource 'as is' to Plugin Framework
First pass at migrating the resource: with this (verbose) groundwork laid the test suite can be fleshed out and the open bugs squashed 🤞🏻 

The current SDKv2 based Trigger resource is being left in place (and its tests skipped) just not wired up for use.
Changing recipient details for Slack type recipient in  honeycombio_trigger causes 422 failure
**Versions**
0.13.0 & 0.14.0

**Steps to reproduce**

1.  Create a simple terraform alert

`main.tf`: 
```
data "honeycombio_query_specification" "query_specification" {
    calculation {
        op = "MAX"
        column = "metrics.cpu.usage"
    }

    filter { 
        column = "label.app"
        op = "="
        value = "jazz-backend-api"
    }
    time_range = 300
}

resource "honeycombio_query" "trigger_query" {
    dataset = "kubernetes-metrics"
    query_json = data.honeycombio_query_specification.query_specification.json
}

resource "honeycombio_trigger" "trigger" {
    name = "honeycomb-bug"
    description = "Created to reproduce observed honeycomb bug"
    query_id = honeycombio_query.trigger_query.id
    dataset = "kubernetes-metrics"
    frequency = 300
    alert_type = "on_change"
    threshold {
        op = ">"
        value = 0.9
    }
    recipient {
      type = "slack"
      target = "#my-alerting-channel"
    } 
}
```

`settings.tf`:
```
terraform { 
    required_providers { 
        honeycombio = {
            source = "honeycombio/honeycombio"
            version = "~> 0.14.0"
        }
    }
    required_version = "~> 1.1.7"
}

provider "honeycombio" {
}
```

Run `terraform init` and `terraform apply` to create alert. Answer prompts for api key, etc. 

2. Modify the target in the recipient block
```
 ...
  recipient {
    type = "slack"
    target = "#my-alerting-channel-2"
  }
```
3. Once again, run `terraform apply` to make changes to the trigger. 

The apply will create a plan
```
Terraform will perform the following actions:

  # honeycombio_trigger.trigger will be updated in-place
  ~ resource "honeycombio_trigger" "trigger" {
        id          = "pweHeSfZh9q"
        name        = "honeycomb-bug"
        # (6 unchanged attributes hidden)

      ~ recipient {
            id     = "m7wahgjr9SC"
          ~ target = "#my-alerting-channel" -> "#my-alerting-channel-2"
            # (1 unchanged attribute hidden)
        }

        # (1 unchanged block hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.
```
but fail when attempting to apply 
```
╷
│ Error: 422 Unprocessable Entity: unknown recipient - check recipient id 'm7wahgjr9SC' values.
│ 
│   with honeycombio_trigger.trigger,
│   on main.tf line 20, in resource "honeycombio_trigger" "trigger":
│   20: resource "honeycombio_trigger" "trigger" {
│ 
╵
```


**Additional context**
Terraform version 1.1.7

I would expect to be able to use the same basic structure to both create triggers with slack recipients, and update the recipient later to go to a new channel if necessary. The issue above poses problems when attempting to use input variables to fill trigger details while creating multiple triggers. Especially when it is not known ahead of time which recipients already exist and which do not. 


Discussion / hints:
## [Codecov](https://app.codecov.io/gh/honeycombio/terraform-provider-honeycombio/pull/306?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=honeycombio) Report
> Merging [#306](https://app.codecov.io/gh/honeycombio/terraform-provider-honeycombio/pull/306?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=honeycombio) (e88ebec) into [main](https://app.codecov.io/gh/honeycombio/terraform-provider-honeycombio/commit/0d9ca767a725485474658a7724ae5bc42bb2efef?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=honeycombio) (0d9ca76) will **decrease** coverage by `5.43%`.
> The diff coverage is `74.67%`.

```diff
@@            Coverage Diff             @@
##             main     #306      +/-   ##
==========================================
- Coverage   80.68%   75.26%   -5.43%     
==========================================
  Files          47       49       +2     
  Lines        3924     4305     +381     
==========================================
+ Hits         3166     3240      +74     
- Misses        624      932     +308     
+ Partials      134      133       -1     
```


| [Impacted Files](https://app.codecov.io/gh/honeycombio/terraform-provider-honeycombio/pull/306?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=honeycombio) | Coverage Δ | |
|---|---|---|
| [honeycombio/provider.go](https://app.codecov.io/gh/honeycombio/terraform-provider-honeycombio/pull/306?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=honeycombio#diff-aG9uZXljb21iaW8vcHJvdmlkZXIuZ28=) | `76.47% <ø> (-0.28%)` | :arrow_down: |
| [internal/provider/trigger\_resource.go](https://app.codecov.io/gh/honeycombio/terraform-provider-honeycombio/pull/306?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=honeycombio#diff-aW50ZXJuYWwvcHJvdmlkZXIvdHJpZ2dlcl9yZXNvdXJjZS5nbw==) | `73.46% <73.46%> (ø)` | |
| [internal/helper/validation/divisible\_by.go](https://app.codecov.io/gh/honeycombio/terraform-provider-honeycombio/pull/306?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=honeycombio#diff-aW50ZXJuYWwvaGVscGVyL3ZhbGlkYXRpb24vZGl2aXNpYmxlX2J5Lmdv) | `88.23% <88.23%> (ø)` | |
| [internal/provider/provider.go](https://app.codecov.io/gh/honeycombio/terraform-provider-honeycombio/pull/306?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=honeycombio#diff-aW50ZXJuYWwvcHJvdmlkZXIvcHJvdmlkZXIuZ28=) | `58.16% <100.00%> (+3.21%)` | :arrow_up: |

... and [2 files with indirect coverage changes](https://app.codecov.io/gh/honeycombio/terraform-provider-honeycombio/pull/306/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=honeycombio)

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=honeycombio)

Thank you for taking the time to report this.

This is another manifestation of the buggy state of these recipient blocks.

Related to #200, #263, and #267

Fix patch:
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
index 8d33c3df..4e7c80d4 100644
--- a/.github/workflows/ci.yaml
+++ b/.github/workflows/ci.yaml
@@ -34,6 +34,8 @@ jobs:
     needs: build
     runs-on: ubuntu-latest
     timeout-minutes: 15
+    env:
+      TERRAFORM_VERSION: "1.0.11"
     steps:
       - name: Checkout
         uses: actions/checkout@v3
@@ -59,8 +61,7 @@ jobs:
 
       - uses: hashicorp/setup-terraform@v2
         with:
-          # TODO: update to a TF version matrix test once testsuite won't generate conflicting resources
-          terraform_version: 0.14
+          terraform_version: ${{ env.TERRAFORM_VERSION }}
           terraform_wrapper: false
 
       - name: Run TF acceptance tests
@@ -69,6 +70,7 @@ jobs:
           HONEYCOMB_API_KEY: ${{ secrets.HONEYCOMB_API_KEY }}
           HONEYCOMB_DATASET: testacc
           TF_ACC: 1
+          TF_ACC_TERRAFORM_VERSION: ${{ env.TERRAFORM_VERSION }}
         run: go test -v -coverprofile=tf-coverage.txt -covermode=atomic ./internal/... ./honeycombio/...
 
       - name: Generate Coverage Report
diff --git a/client/recipient.go b/client/recipient.go
index 31d700a3..5a19d379 100644
--- a/client/recipient.go
+++ b/client/recipient.go
@@ -17,13 +17,13 @@ type Recipients interface {
 	// the given ID
 	Get(ctx context.Context, id string) (*Recipient, error)
 
-	// Create a new Recipient. When creating a new Recipient ID ma not be set.
+	// Create a new Recipient. When creating a new Recipient ID must not be set.
 	Create(ctx context.Context, r *Recipient) (*Recipient, error)
 
 	// Update an existing Recipient.
 	Update(ctx context.Context, r *Recipient) (*Recipient, error)
 
-	// Delete a Recipient from the dataset.
+	// Delete a Recipient
 	Delete(ctx context.Context, id string) error
 }
 
@@ -92,6 +92,7 @@ const (
 	PDSeverityERROR    PagerDutySeverity = "error"
 	PDSeverityWARNING  PagerDutySeverity = "warning"
 	PDSeverityINFO     PagerDutySeverity = "info"
+	PDDefaultSeverity                    = PDSeverityCRITICAL
 )
 
 // TriggerRecipientTypes returns a list of recipient types compatible with Triggers
diff --git a/internal/helper/modifiers/notification_recipients.go b/internal/helper/modifiers/notification_recipients.go
new file mode 100644
index 00000000..11dc6f87
--- /dev/null
+++ b/internal/helper/modifiers/notification_recipients.go
@@ -0,0 +1,70 @@
+package modifiers
+
+import (
+	"context"
+
+	"github.com/hashicorp/terraform-plugin-framework/path"
+	"github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
+	"github.com/hashicorp/terraform-plugin-framework/types"
+
+	"github.com/honeycombio/terraform-provider-honeycombio/client"
+	"github.com/honeycombio/terraform-provider-honeycombio/internal/models"
+)
+
+type notificationRecipientsModifier struct{}
+
+var _ planmodifier.Set = &notificationRecipientsModifier{}
+
+func (m notificationRecipientsModifier) Description(_ context.Context) string {
+	return "Handles the default states and value manipulations for notificiation recipients."
+}
+
+func (m notificationRecipientsModifier) MarkdownDescription(ctx context.Context) string {
+	return m.Description(ctx)
+}
+
+func (m notificationRecipientsModifier) PlanModifySet(ctx context.Context, req planmodifier.SetRequest, resp *planmodifier.SetResponse) {
+	// Do nothing on resource destroy.
+	if req.Plan.Raw.IsNull() {
+		return
+	}
+
+	var plan models.TriggerResourceModel
+	req.Plan.Get(ctx, &plan)
+
+	var rcpts []models.NotificationRecipientModel
+	req.Plan.GetAttribute(ctx, path.Root("recipient"), &rcpts)
+
+	// manage null values properly based on the type of recipient
+	if !req.StateValue.IsNull() {
+		for i, r := range rcpts {
+			if r.ID.IsUnknown() && r.Type.IsUnknown() {
+				// likely dependant on creation of another resource
+				continue
+			}
+			if r.ID.IsUnknown() && !r.Type.IsUnknown() {
+				// specified by type and target
+				r.ID = types.StringNull()
+				if r.Type.ValueString() == string(client.RecipientTypePagerDuty) {
+					// PagerDuty recipients do not have a target
+					r.Target = types.StringNull()
+				}
+			}
+			if !r.ID.IsUnknown() && r.Type.IsUnknown() {
+				// specified by ID
+				r.Type = types.StringNull()
+				r.Target = types.StringNull()
+			}
+
+			rcpts[i] = r
+		}
+
+		updated, diag := types.SetValueFrom(ctx, req.PlanValue.ElementType(ctx), rcpts)
+		resp.Diagnostics.Append(diag...)
+		resp.PlanValue = updated
+	}
+}
+
+func NotificationRecipients() planmodifier.Set {
+	return notificationRecipientsModifier{}
+}
diff --git a/internal/provider/trigger_resource.go b/internal/provider/trigger_resource.go
index 70b0f5e3..1f846f76 100644
--- a/internal/provider/trigger_resource.go
+++ b/internal/provider/trigger_resource.go
@@ -5,6 +5,8 @@ import (
 	"errors"
 	"strings"
 
+	"golang.org/x/exp/slices"
+
 	"github.com/hashicorp/terraform-plugin-framework-validators/int64validator"
 	"github.com/hashicorp/terraform-plugin-framework-validators/listvalidator"
 	"github.com/hashicorp/terraform-plugin-framework-validators/objectvalidator"
@@ -14,6 +16,7 @@ import (
 	"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/listplanmodifier"
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringdefault"
 	"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier"
@@ -22,6 +25,7 @@ import (
 
 	"github.com/honeycombio/terraform-provider-honeycombio/client"
 	"github.com/honeycombio/terraform-provider-honeycombio/internal/helper"
+	"github.com/honeycombio/terraform-provider-honeycombio/internal/helper/modifiers"
 	"github.com/honeycombio/terraform-provider-honeycombio/internal/helper/validation"
 	"github.com/honeycombio/terraform-provider-honeycombio/internal/models"
 )
@@ -137,8 +141,9 @@ func (r *triggerResource) Schema(_ context.Context, _ resource.SchemaRequest, re
 					},
 				},
 			},
-			"recipient": schema.ListNestedBlock{
-				Description: "Zero or more recipients to notify when the Trigger fires.",
+			"recipient": schema.SetNestedBlock{
+				Description:   "Zero or more recipients to notify when the Trigger fires.",
+				PlanModifiers: []planmodifier.Set{modifiers.NotificationRecipients()},
 				NestedObject: schema.NestedBlockObject{
 					Validators: []validator.Object{
 						objectvalidator.AtLeastOneOf(
@@ -179,13 +184,14 @@ func (r *triggerResource) Schema(_ context.Context, _ resource.SchemaRequest, re
 							Validators: []validator.List{
 								listvalidator.SizeAtMost(1),
 							},
+							PlanModifiers: []planmodifier.List{listplanmodifier.UseStateForUnknown()},
 							NestedObject: schema.NestedBlockObject{
 								Attributes: map[string]schema.Attribute{
 									"pagerduty_severity": schema.StringAttribute{
-										Optional:    true,
-										Computed:    true,
-										Description: "The severity to set with the PagerDuty notification.",
-										Default:     stringdefault.StaticString("critical"),
+										Optional:      true,
+										Computed:      true,
+										Description:   "The severity to set with the PagerDuty notification. If no severity is provided, 'critical' is assumed.",
+										PlanModifiers: []planmodifier.String{stringplanmodifier.UseStateForUnknown()},
 										Validators: []validator.String{
 											stringvalidator.All(
 												stringvalidator.OneOf("info", "warning", "error", "critical"),
@@ -208,13 +214,12 @@ func (r *triggerResource) Configure(_ context.Context, req resource.ConfigureReq
 
 func (r *triggerResource) Create(ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse) {
 	var plan models.TriggerResourceModel
-	diags := req.Plan.Get(ctx, &plan)
-	resp.Diagnostics.Append(diags...)
+	resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)
 	if resp.Diagnostics.HasError() {
 		return
 	}
 
-	newTrigger := &client.Trigger{
+	trigger, err := r.client.Triggers.Create(ctx, plan.Dataset.ValueString(), &client.Trigger{
 		Name:        plan.Name.ValueString(),
 		Description: plan.Description.ValueString(),
 		Disabled:    plan.Disabled.ValueBool(),
@@ -223,9 +228,7 @@ func (r *triggerResource) Create(ctx context.Context, req resource.CreateRequest
 		Threshold:   expandTriggerThreshold(plan.Threshold),
 		Frequency:   int(plan.Frequency.ValueInt64()),
 		Recipients:  expandNotificationRecipients(plan.Recipients),
-	}
-
-	trigger, err := r.client.Triggers.Create(ctx, plan.Dataset.ValueString(), newTrigger)
+	})
 	if err != nil {
 		resp.Diagnostics.AddError(
 			"Error Creating Honeycomb Trigger",
@@ -234,24 +237,68 @@ func (r *triggerResource) Create(ctx context.Context, req resource.CreateRequest
 		return
 	}
 
-	plan.ID = types.StringValue(trigger.ID)
-	plan.Name = types.StringValue(trigger.Name)
-	plan.Description = types.StringValue(trigger.Description)
-	plan.Disabled = types.BoolValue(trigger.Disabled)
-	plan.QueryID = types.StringValue(trigger.QueryID)
-	plan.AlertType = types.StringValue(trigger.AlertType)
-	plan.Threshold = flattenTriggerThreshold(trigger.Threshold)
-	plan.Frequency = types.Int64Value(int64(trigger.Frequency))
-	plan.Recipients = flattenNotificationRecipients(trigger.Recipients)
-
-	diags = resp.State.Set(ctx, plan)
-	resp.Diagnostics.Append(diags...)
+	var state models.TriggerResourceModel
+	state.Dataset = plan.Dataset
+	state.ID = types.StringValue(trigger.ID)
+	state.Name = types.StringValue(trigger.Name)
+	state.Description = types.StringValue(trigger.Description)
+	state.Disabled = types.BoolValue(trigger.Disabled)
+	state.QueryID = types.StringValue(trigger.QueryID)
+	state.AlertType = types.StringValue(trigger.AlertType)
+	state.Threshold = flattenTriggerThreshold(trigger.Threshold)
+	state.Frequency = types.Int64Value(int64(trigger.Frequency))
+
+	recipients := make([]models.NotificationRecipientModel, len(trigger.Recipients))
+	for i, r := range trigger.Recipients {
+		var rcpt models.NotificationRecipientModel
+
+		// match the trigger's recipient to that in the plan
+		idx := slices.IndexFunc(plan.Recipients, func(s models.NotificationRecipientModel) bool {
+			if !s.ID.IsUnknown() {
+				return s.ID.ValueString() == r.ID
+			}
+			return s.Type.ValueString() == string(r.Type) && (s.Target.IsNull() || s.Target.ValueString() == r.Target)
+		})
+		if idx < 0 {
+			resp.Diagnostics.AddError(
+				"Error Creating Honeycomb Trigger",
+				"Could not find Recipient "+r.ID+" in plan",
+			)
+		}
+		rcpt = plan.Recipients[idx]
+
+		// TODO: can we move this to the planmodifier by adding a create state?
+		if !rcpt.ID.IsUnknown() {
+			// recipient provided by ID
+			rcpt.ID = types.StringValue(r.ID)
+			rcpt.Type = types.StringNull()
+			rcpt.Target = types.StringNull()
+		} else {
+			// recipient provided by type+target
+			rcpt.ID = types.StringNull()
+			rcpt.Type = types.StringValue(string(r.Type))
+			if rcpt.Type.ValueString() == string(client.RecipientTypePagerDuty) {
+				// PagerDuty recipients don't have a target
+				rcpt.Target = types.StringNull()
+			} else {
+				rcpt.Target = types.StringValue(r.Target)
+			}
+		}
+
+		if r.Type == client.RecipientTypePagerDuty && r.Details != nil {
+			rcpt.Details = make([]models.NotificationRecipientDetailsModel, 1)
+			rcpt.Details[0].PDSeverity = types.StringValue(string(r.Details.PDSeverity))
+		}
+		recipients[i] = rcpt
+	}
+	state.Recipients = recipients
+
+	resp.Diagnostics.Append(resp.State.Set(ctx, state)...)
 }
 
 func (r *triggerResource) Read(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) {
 	var state models.TriggerResourceModel
-	diags := req.State.Get(ctx, &state)
-	resp.Diagnostics.Append(diags...)
+	resp.Diagnostics.Append(req.State.Get(ctx, &state)...)
 	if resp.Diagnostics.HasError() {
 		return
 	}
@@ -276,21 +323,43 @@ func (r *triggerResource) Read(ctx context.Context, req resource.ReadRequest, re
 	state.AlertType = types.StringValue(trigger.AlertType)
 	state.Threshold = flattenTriggerThreshold(trigger.Threshold)
 	state.Frequency = types.Int64Value(int64(trigger.Frequency))
-	state.Recipients = flattenNotificationRecipients(trigger.Recipients)
 
-	diags = resp.State.Set(ctx, &state)
-	resp.Diagnostics.Append(diags...)
+	recipients := make([]models.NotificationRecipientModel, len(trigger.Recipients))
+	if state.Recipients != nil {
+		for i, r := range trigger.Recipients {
+			// match the Trigger's recipient to that in state
+			idx := slices.IndexFunc(state.Recipients, func(s models.NotificationRecipientModel) bool {
+				if !s.ID.IsNull() {
+					return s.ID.ValueString() == r.ID
+				}
+				return s.Type.ValueString() == string(r.Type) && (s.Target.IsNull() || s.Target.ValueString() == r.Target)
+			})
+			if idx < 0 {
+				resp.Diagnostics.AddError(
+					"Error Reading Honeycomb Trigger",
+					"Could not find Recipient "+r.ID+" in state",
+				)
+			}
+
+			recipients[i] = state.Recipients[idx]
+		}
+	} else {
+		recipients = flattenNotificationRecipients(trigger.Recipients)
+	}
+	state.Recipients = recipients
+
+	resp.Diagnostics.Append(resp.State.Set(ctx, state)...)
 }
 
 func (r *triggerResource) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) {
 	var plan models.TriggerResourceModel
-	diags := req.Plan.Get(ctx, &plan)
-	resp.Diagnostics.Append(diags...)
+	resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)
 	if resp.Diagnostics.HasError() {
 		return
 	}
 
-	newTrigger := &client.Trigger{
+	_, err := r.client.Triggers.Update(ctx, plan.Dataset.ValueString(), &client.Trigger{
+		ID:          plan.ID.ValueString(),
 		Name:        plan.Name.ValueString(),
 		Description: plan.Description.ValueString(),
 		Disabled:    plan.Disabled.ValueBool(),
@@ -299,9 +368,7 @@ func (r *triggerResource) Update(ctx context.Context, req resource.UpdateRequest
 		Frequency:   int(plan.Frequency.ValueInt64()),
 		Threshold:   expandTriggerThreshold(plan.Threshold),
 		Recipients:  expandNotificationRecipients(plan.Recipients),
-	}
-
-	_, err := r.client.Triggers.Update(ctx, plan.Dataset.ValueString(), newTrigger)
+	})
 	if err != nil {
 		resp.Diagnostics.AddError(
 			"Error Updating Honeycomb Trigger",
@@ -313,36 +380,48 @@ func (r *triggerResource) Update(ctx context.Context, req resource.UpdateRequest
 	trigger, err := r.client.Triggers.Get(ctx, plan.Dataset.ValueString(), plan.ID.ValueString())
 	if err != nil {
 		resp.Diagnostics.AddError(
-			"Error Reading Honeycomb Trigger",
+			"Error Updating Honeycomb Trigger",
 			"Could not read Honeycomb Trigger ID "+plan.ID.ValueString()+": "+err.Error(),
 		)
 		return
 	}
 
-	plan.ID = types.StringValue(trigger.ID)
-	plan.Name = types.StringValue(trigger.Name)
-	plan.Description = types.StringValue(trigger.Description)
-	plan.Disabled = types.BoolValue(trigger.Disabled)
-	plan.QueryID = types.StringValue(trigger.QueryID)
-	plan.AlertType = types.StringValue(trigger.AlertType)
-	plan.Frequency = types.Int64Value(int64(trigger.Frequency))
-	plan.Threshold = []models.TriggerThresholdModel{{
-		Op:    types.StringValue(string(trigger.Threshold.Op)),
-		Value: types.Float64Value(trigger.Threshold.Value),
-	}}
-	plan.Recipients = flattenNotificationRecipients(trigger.Recipients)
+	var state models.TriggerResourceModel
+	state.Dataset = plan.Dataset
+	state.ID = types.StringValue(trigger.ID)
+	state.Name = types.StringValue(trigger.Name)
+	state.Description = types.StringValue(trigger.Description)
+	state.Disabled = types.BoolValue(trigger.Disabled)
+	state.QueryID = types.StringValue(trigger.QueryID)
+	state.AlertType = types.StringValue(trigger.AlertType)
+	state.Frequency = types.Int64Value(int64(trigger.Frequency))
+	state.Threshold = flattenTriggerThreshold(trigger.Threshold)
 
-	diags = resp.State.Set(ctx, plan)
-	resp.Diagnostics.Append(diags...)
-	if resp.Diagnostics.HasError() {
-		return
+	recipients := make([]models.NotificationRecipientModel, len(trigger.Recipients))
+	for i, r := range trigger.Recipients {
+		// match the Trigger's recipient to that in the plan
+		idx := slices.IndexFunc(plan.Recipients, func(s models.NotificationRecipientModel) bool {
+			if !s.ID.IsNull() {
+				return s.ID.ValueString() == r.ID
+			}
+			return s.Type.ValueString() == string(r.Type) && (s.Target.IsNull() || s.Target.ValueString() == r.Target)
+		})
+		if idx < 0 {
+			resp.Diagnostics.AddError(
+				"Error Updating Honeycomb Trigger",
+				"Could not find Recipient "+r.ID+" in plan",
+			)
+		}
+		recipients[i] = plan.Recipients[idx]
 	}
+	state.Recipients = recipients
+
+	resp.Diagnostics.Append(resp.State.Set(ctx, state)...)
 }
 
 func (r *triggerResource) Delete(ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse) {
 	var state models.TriggerResourceModel
-	diags := req.State.Get(ctx, &state)
-	resp.Diagnostics.Append(diags...)
+	resp.Diagnostics.Append(req.State.Get(ctx, &state)...)
 	if resp.Diagnostics.HasError() {
 		return
 	}
@@ -405,7 +484,7 @@ func expandNotificationRecipients(n []models.NotificationRecipientModel) []clien
 			Type:   client.RecipientType(r.Type.ValueString()),
 			Target: r.Target.ValueString(),
 		}
-		if len(r.Details) == 1 {
+		if r.Details != nil {
 			rcpt.Details = &client.NotificationRecipientDetails{
 				PDSeverity: client.PagerDutySeverity(r.Details[0].PDSeverity.ValueString()),
 			}


Test patch:
diff --git a/honeycombio/data_source_column_test.go b/honeycombio/data_source_column_test.go
index 0d1ee78b..ca8238dd 100644
--- a/honeycombio/data_source_column_test.go
+++ b/honeycombio/data_source_column_test.go
@@ -8,6 +8,8 @@ import (
 	"testing"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+	"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
+
 	honeycombio "github.com/honeycombio/terraform-provider-honeycombio/client"
 )
 
@@ -18,7 +20,7 @@ func TestAccDataSourceHoneycombioColumn_basic(t *testing.T) {
 
 	testColumns := []honeycombio.Column{
 		{
-			KeyName:     "test_column3",
+			KeyName:     acctest.RandString(4) + "_test_column3",
 			Description: "test column3",
 			Type:        honeycombio.ToPtr(honeycombio.ColumnType("float")),
 		},
@@ -45,7 +47,7 @@ func TestAccDataSourceHoneycombioColumn_basic(t *testing.T) {
 		Steps: []resource.TestStep{
 			// match by name and return a single column with the right type
 			{
-				Config: testAccDataSourceColumnConfig([]string{"dataset = \"" + testAccDataset() + "\"", "name = \"test_column3\""}),
+				Config: testAccDataSourceColumnConfig([]string{"dataset = \"" + testAccDataset() + "\"", "name = \"" + testColumns[0].KeyName + "\""}),
 				Check:  resource.TestCheckResourceAttr("data.honeycombio_column.test", "type", "float"),
 			},
 			// test a failed match
diff --git a/honeycombio/data_source_columns_test.go b/honeycombio/data_source_columns_test.go
index 9e3f4fb7..d863ed4f 100644
--- a/honeycombio/data_source_columns_test.go
+++ b/honeycombio/data_source_columns_test.go
@@ -7,6 +7,8 @@ import (
 	"testing"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+	"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
+
 	honeycombio "github.com/honeycombio/terraform-provider-honeycombio/client"
 )
 
@@ -15,13 +17,15 @@ func TestAccDataSourceHoneycombioColumns_basic(t *testing.T) {
 	c := testAccClient(t)
 	dataset := testAccDataset()
 
+	testprefix := acctest.RandString(4)
+
 	testColumns := []honeycombio.Column{
 		{
-			KeyName:     "test_column1",
+			KeyName:     testprefix + "_test_column1",
 			Description: "test column1",
 		},
 		{
-			KeyName:     "test_column2",
+			KeyName:     testprefix + "_test_column2",
 			Description: "test column2",
 		},
 	}
@@ -52,7 +56,7 @@ func TestAccDataSourceHoneycombioColumns_basic(t *testing.T) {
 				),
 			},
 			{
-				Config: testAccDataSourceColumnsConfig([]string{"dataset = \"" + testAccDataset() + "\"", "starts_with = \"test_column\""}),
+				Config: testAccDataSourceColumnsConfig([]string{"dataset = \"" + testAccDataset() + "\"", "starts_with = \"" + testprefix + "\""}),
 				Check:  resource.TestCheckResourceAttr("data.honeycombio_columns.test", "names.#", "2"),
 			},
 			{
diff --git a/internal/provider/derived_column_data_source_test.go b/internal/provider/derived_column_data_source_test.go
index f61949f9..40ee83cb 100644
--- a/internal/provider/derived_column_data_source_test.go
+++ b/internal/provider/derived_column_data_source_test.go
@@ -7,6 +7,7 @@ import (
 
 	"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-testing/helper/resource"
+
 	"github.com/honeycombio/terraform-provider-honeycombio/client"
 )
 
diff --git a/internal/provider/trigger_resource_test.go b/internal/provider/trigger_resource_test.go
index b6568f9f..052ed93d 100644
--- a/internal/provider/trigger_resource_test.go
+++ b/internal/provider/trigger_resource_test.go
@@ -5,8 +5,11 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/hashicorp/terraform-plugin-testing/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-testing/helper/resource"
 	"github.com/hashicorp/terraform-plugin-testing/terraform"
+
+	"github.com/honeycombio/terraform-provider-honeycombio/client"
 )
 
 func TestAcc_TriggerResource(t *testing.T) {
@@ -17,7 +20,7 @@ func TestAcc_TriggerResource(t *testing.T) {
 		ProtoV5ProviderFactories: testAccProtoV5MuxServerFactory,
 		Steps: []resource.TestStep{
 			{
-				Config: basicTriggerTestConfig(dataset),
+				Config: testAccConfigBasicTriggerTest(dataset, "info"),
 				Check: resource.ComposeAggregateTestCheckFunc(
 					testAccEnsureTriggerExists(t, "honeycombio_trigger.test"),
 					resource.TestCheckResourceAttr("honeycombio_trigger.test", "name", "Test trigger from terraform-provider-honeycombio"),
@@ -26,11 +29,14 @@ func TestAcc_TriggerResource(t *testing.T) {
 					resource.TestCheckResourceAttrPair("honeycombio_trigger.test", "query_id", "honeycombio_query.test", "id"),
 				),
 			},
+			// then update the PD Severity from info -> critical (the default)
+			{
+				Config: testAccConfigBasicTriggerTest(dataset, "critical"),
+			},
 			{
 				ResourceName:        "honeycombio_trigger.test",
 				ImportStateIdPrefix: fmt.Sprintf("%v/", dataset),
 				ImportState:         true,
-				ImportStateVerify:   true,
 			},
 		},
 	})
@@ -40,13 +46,12 @@ func TestAcc_TriggerResource(t *testing.T) {
 // case from the last SDK-based version of the Trigger resource to the current Framework-based
 // version.
 //
-// State is first generated with the SDKv2 provider and then a plan is done with the new provider to
-// ensure there are no planned changes after migrating to the Framework-based resource.
-//
 // See: https://developer.hashicorp.com/terraform/plugin/framework/migrating/testing#testing-migration
 func TestAcc_TriggerResourceUpgradeFromVersion014(t *testing.T) {
 	dataset := testAccDataset()
 
+	config := testAccConfigBasicTriggerTest(dataset, "info")
+
 	resource.Test(t, resource.TestCase{
 		Steps: []resource.TestStep{
 			{
@@ -56,22 +61,199 @@ func TestAcc_TriggerResourceUpgradeFromVersion014(t *testing.T) {
 						Source:            "honeycombio/honeycombio",
 					},
 				},
-				Config: basicTriggerTestConfig(dataset),
+				Config: config,
 				Check: resource.ComposeTestCheckFunc(
 					testAccEnsureTriggerExists(t, "honeycombio_trigger.test"),
 				),
 			},
 			{
 				ProtoV5ProviderFactories: testAccProtoV5MuxServerFactory,
-				Config:                   basicTriggerTestConfig(dataset),
-				PlanOnly:                 true,
+				Config:                   config,
 			},
 		},
 	})
 }
 
-func basicTriggerTestConfig(dataset string) string {
-	return fmt.Sprintf(`
+func TestAcc_TriggerResourceUpdateRecipientByID(t *testing.T) {
+	ctx := context.Background()
+	c := testAccClient(t)
+	dataset := testAccDataset()
+
+	testRecipients := []client.Recipient{
+		{
+			Type: client.RecipientTypeEmail,
+			Details: client.RecipientDetails{
+				EmailAddress: acctest.RandString(8) + "@example.com",
+			},
+		},
+		{
+			Type: client.RecipientTypeSlack,
+			Details: client.RecipientDetails{
+				SlackChannel: "#" + acctest.RandString(8),
+			},
+		},
+	}
+
+	for i, r := range testRecipients {
+		rcpt, err := c.Recipients.Create(ctx, &r)
+		if err != nil {
+			t.Error(err)
+		}
+		// update ID for removal later
+		testRecipients[i].ID = rcpt.ID
+	}
+	t.Cleanup(func() {
+		// remove recipients at the of the test run
+		for _, col := range testRecipients {
+			//nolint:errcheck
+			c.DerivedColumns.Delete(ctx, dataset, col.ID)
+		}
+	})
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:                 testAccPreCheck(t),
+		ProtoV5ProviderFactories: testAccProtoV5MuxServerFactory,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccConfigTriggerRecipientByID(dataset, testRecipients[0].ID),
+				Check:  resource.TestCheckResourceAttr("honeycombio_trigger.test", "recipient.0.id", testRecipients[0].ID),
+			},
+			{
+				Config: testAccConfigTriggerRecipientByID(dataset, testRecipients[1].ID),
+				Check:  resource.TestCheckResourceAttr("honeycombio_trigger.test", "recipient.0.id", testRecipients[1].ID),
+			},
+		},
+	})
+}
+
+func TestAcc_TriggerResourceRecipientOrderingStable(t *testing.T) {
+	dataset := testAccDataset()
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:                 testAccPreCheck(t),
+		ProtoV5ProviderFactories: testAccProtoV5MuxServerFactory,
+		Steps: []resource.TestStep{
+			{
+				Config: fmt.Sprintf(`
+data "honeycombio_query_specification" "test" {
+  calculation {
+    op     = "COUNT"
+  }
+
+  time_range = 1200
+}
+
+resource "honeycombio_query" "test" {
+  dataset    = "%[1]s"
+  query_json = data.honeycombio_query_specification.test.json
+}
+
+resource "honeycombio_trigger" "test" {
+  name = "Ensure mixed order recipients don't cause infinite diffs"
+
+  query_id = honeycombio_query.test.id
+  dataset  = "%[1]s"
+
+  threshold {
+    op    = ">"
+    value = 1000
+  }
+
+  alert_type = "on_change"
+
+  recipient {
+    type   = "slack"
+    target = "#test2"
+  }
+
+  recipient {
+    type   = "slack"
+    target = "#test"
+  }
+
+  recipient {
+    type   = "email"
+    target = "bob@example.com"
+  }
+
+  recipient {
+    type   = "email"
+    target = "alice@example.com"
+  }
+
+  recipient {
+    type   = "marker"
+    target = "trigger fired"
+  }
+}
+`, dataset),
+			},
+			{
+				// now remove two recipients and add a new one
+				Config: fmt.Sprintf(`
+data "honeycombio_query_specification" "test" {
+  calculation {
+    op     = "COUNT"
+  }
+
+  time_range = 1200
+}
+
+resource "honeycombio_query" "test" {
+  dataset    = "%[1]s"
+  query_json = data.honeycombio_query_specification.test.json
+}
+
+resource "honeycombio_trigger" "test" {
+  name = "Ensure mixed order recipients don't cause infinite diffs"
+
+  query_id = honeycombio_query.test.id
+  dataset  = "%[1]s"
+
+  threshold {
+    op    = ">"
+    value = 1000
+  }
+
+  alert_type = "on_change"
+
+  recipient {
+    type   = "slack"
+    target = "#test"
+  }
+
+  recipient {
+    type   = "email"
+    target = "bob@example.com"
+  }
+
+  recipient {
+    type   = "email"
+    target = "alice@example.com"
+  }
+
+  recipient {
+    type   = "slack"
+    target = "#a-new-channel"
+  }
+}
+`, dataset),
+			},
+		},
+	})
+}
+
+func TestAcc_TriggerResourcePagerDutyUnsetSeverity(t *testing.T) {
+	t.Skip("known issue: see https://github.com/honeycombio/terraform-provider-honeycombio/issues/309")
+
+	dataset := testAccDataset()
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:                 testAccPreCheck(t),
+		ProtoV5ProviderFactories: testAccProtoV5MuxServerFactory,
+		Steps: []resource.TestStep{
+			{
+				Config: fmt.Sprintf(`
 data "honeycombio_query_specification" "test" {
   calculation {
     op     = "AVG"
@@ -87,7 +269,52 @@ resource "honeycombio_query" "test" {
 
 resource "honeycombio_pagerduty_recipient" "test" {
   integration_key  = "09c9d4cacd68933151a1ef1048b67dd5"
-  integration_name = "acctest"
+  integration_name = "severity-test"
+}
+
+resource "honeycombio_trigger" "test" {
+  name    = "Test trigger from terraform-provider-honeycombio"
+  dataset = "%[1]s"
+
+  description = "My nice description"
+
+  query_id = honeycombio_query.test.id
+
+  threshold {
+    op    = ">"
+    value = 100
+  }
+
+  frequency = data.honeycombio_query_specification.test.time_range / 2
+
+  recipient {
+    id = honeycombio_pagerduty_recipient.test.id
+    // default severity is 'critical'
+  }
+}`, dataset),
+			},
+		},
+	})
+}
+
+func testAccConfigBasicTriggerTest(dataset, pdseverity string) string {
+	return fmt.Sprintf(`
+data "honeycombio_query_specification" "test" {
+  calculation {
+    op     = "AVG"
+    column = "duration_ms"
+  }
+  time_range = 1200
+}
+
+resource "honeycombio_query" "test" {
+  dataset    = "%[1]s"
+  query_json = data.honeycombio_query_specification.test.json
+}
+
+resource "honeycombio_pagerduty_recipient" "test" {
+  integration_key  = "08b9d4cacd68933151a1ef1028b67da2"
+  integration_name = "testacc-basic"
 }
 
 resource "honeycombio_trigger" "test" {
@@ -114,10 +341,41 @@ resource "honeycombio_trigger" "test" {
     id = honeycombio_pagerduty_recipient.test.id
 
     notification_details {
-      pagerduty_severity = "info"
+      pagerduty_severity = "%[2]s"
     }
   }
-}`, dataset)
+}`, dataset, pdseverity)
+}
+
+func testAccConfigTriggerRecipientByID(dataset, recipientID string) string {
+	return fmt.Sprintf(`
+data "honeycombio_query_specification" "test" {
+  calculation {
+    op     = "AVG"
+    column = "duration_ms"
+  }
+
+  time_range = 1800
+}
+
+resource "honeycombio_query" "test" {
+  dataset    = "%[1]s"
+  query_json = data.honeycombio_query_specification.test.json
+}
+
+resource "honeycombio_trigger" "test" {
+  name    = "Test trigger with changing recipient id"
+  dataset = "%[1]s"
+  query_id = honeycombio_query.test.id
+  threshold {
+    op    = ">"
+    value = 100
+  }
+
+  recipient {
+    id = "%[2]s"
+  }
+}`, dataset, recipientID)
 }
 
 func testAccEnsureTriggerExists(t *testing.T, name string) resource.TestCheckFunc {