Skip to content

Commit 5d1d8ac

Browse files
elimtelimt
authored
fix(r/slo): SLO update causing resource replacement (#710)
Updating SLO resource after recreation cause the resource to be completely replaced. This was due to `RequiresReplace` forcing a new update everytime. - add a new plan modifier that only requires the SLO resource to be replaced when `datasets` is changed - add `UseStateForUnknown` which copies the prior state value, if not null. This will address the bug where the SLO cannot be found during updates <!-- Thank you for contributing to the project! 💜 Please see our [OSS process document](https://github.com/honeycombio/home/blob/main/honeycomb-oss-lifecycle-and-practices.md#) to get an idea of how we operate. --> ## Which problem is this PR solving? - #701 ## Short description of the changes Updating SLO resource after recreation cause the resource to be completely replaced. This was due to `RequiresReplace` forcing a new update everytime. - add a new plan modifier that only requires the SLO resource to be replaced when `datasets` is changed - add `UseStateForUnknown` which copies the prior state value, if not null. This will address the bug where the SLO cannot be found during updates ## How to verify that this has the expected result 1. Create a SLO resource and note the ID 2. Change any field in the SLO and note the ID 3. The ID should now be different
1 parent 9e03699 commit 5d1d8ac

File tree

3 files changed

+184
-2
lines changed

3 files changed

+184
-2
lines changed

‎internal/helper/modifiers/datasets_require_replace.go

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
package modifiers
2+
3+
import (
4+
"context"
5+
6+
"github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
7+
"github.com/hashicorp/terraform-plugin-framework/types"
8+
)
9+
10+
// RequiresReplaceIfDatasetChanges returns a plan modifier that forces resource replacement
11+
// when the datasets field changes.
12+
func RequiresReplaceIfDatasetChanges() planmodifier.Set {
13+
return requiresReplaceIfDatasetChangesSetModifier{}
14+
}
15+
16+
var _ planmodifier.Set = &requiresReplaceIfDatasetChangesSetModifier{}
17+
18+
type requiresReplaceIfDatasetChangesSetModifier struct{}
19+
20+
func (r requiresReplaceIfDatasetChangesSetModifier) Description(ctx context.Context) string {
21+
return "If the datasets value changes, Terraform will destroy and recreate the resource."
22+
}
23+
24+
func (r requiresReplaceIfDatasetChangesSetModifier) MarkdownDescription(ctx context.Context) string {
25+
return r.Description(ctx)
26+
}
27+
28+
func (r requiresReplaceIfDatasetChangesSetModifier) PlanModifySet(ctx context.Context, req planmodifier.SetRequest, resp *planmodifier.SetResponse) {
29+
// Don't require replacement during create
30+
if req.State.Raw.IsNull() {
31+
return
32+
}
33+
34+
// Don't require replacement during destroy
35+
if req.Plan.Raw.IsNull() {
36+
return
37+
}
38+
39+
var stateValue, planValue types.Set
40+
41+
resp.Diagnostics.Append(req.State.GetAttribute(ctx, req.Path, &stateValue)...)
42+
if resp.Diagnostics.HasError() {
43+
return
44+
}
45+
46+
resp.Diagnostics.Append(req.Plan.GetAttribute(ctx, req.Path, &planValue)...)
47+
if resp.Diagnostics.HasError() {
48+
return
49+
}
50+
51+
// If both are null, no change
52+
if stateValue.IsNull() && planValue.IsNull() {
53+
return
54+
}
55+
56+
// If both are unknown, can't determine change
57+
if stateValue.IsUnknown() || planValue.IsUnknown() {
58+
return
59+
}
60+
61+
// If values are equal, no change
62+
if stateValue.Equal(planValue) {
63+
return
64+
}
65+
66+
// If we get here, the datasets value has changed - require replacement
67+
resp.RequiresReplace = true
68+
}

‎internal/provider/slo_resource.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import (
1414
"github.com/hashicorp/terraform-plugin-framework/resource"
1515
"github.com/hashicorp/terraform-plugin-framework/resource/schema"
1616
"github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
17-
"github.com/hashicorp/terraform-plugin-framework/resource/schema/setplanmodifier"
17+
"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier"
1818
"github.com/hashicorp/terraform-plugin-framework/schema/validator"
1919
"github.com/hashicorp/terraform-plugin-framework/types"
2020

@@ -64,6 +64,9 @@ func (*sloResource) Schema(_ context.Context, _ resource.SchemaRequest, resp *re
6464
"id": schema.StringAttribute{
6565
Description: "The ID of the SLO.",
6666
Computed: true,
67+
PlanModifiers: []planmodifier.String{
68+
stringplanmodifier.UseStateForUnknown(),
69+
},
6770
},
6871
"name": schema.StringAttribute{
6972
Description: "The name of the SLO.",
@@ -96,7 +99,7 @@ func (*sloResource) Schema(_ context.Context, _ resource.SchemaRequest, resp *re
9699
Computed: true,
97100
ElementType: types.StringType,
98101
PlanModifiers: []planmodifier.Set{
99-
setplanmodifier.RequiresReplace(),
102+
modifiers.RequiresReplaceIfDatasetChanges(),
100103
},
101104
Validators: []validator.Set{
102105
setvalidator.SizeAtLeast(1),

‎internal/provider/slo_resource_test.go

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,117 @@ func TestAccHoneycombioSLO_DatasetConstraint(t *testing.T) {
317317
})
318318
}
319319

320+
func TestAccHoneycombioSLO_Update(t *testing.T) {
321+
c := testAccClient(t)
322+
if c.IsClassic(context.Background()) {
323+
t.Skip("Multi-dataset SLOs are not supported in classic")
324+
}
325+
326+
dataset1, dataset2, sliAlias := mdSLOAccTestSetup(t)
327+
328+
slo := &client.SLO{}
329+
var originalID string
330+
331+
resource.Test(t, resource.TestCase{
332+
PreCheck: testAccPreCheck(t),
333+
ProtoV5ProviderFactories: testAccProtoV5ProviderFactory,
334+
Steps: []resource.TestStep{
335+
{
336+
Config: fmt.Sprintf(`
337+
resource "honeycombio_slo" "test" {
338+
name = "TestAcc SLO"
339+
description = "test SLO"
340+
datasets = ["%s"]
341+
sli = "%s"
342+
target_percentage = 99.95
343+
time_period = 30
344+
345+
tags = {
346+
env = "test"
347+
team = "blue"
348+
}
349+
}`, dataset1.Slug, sliAlias.Alias),
350+
Check: resource.ComposeTestCheckFunc(
351+
testAccCheckSLOExists(t, "honeycombio_slo.test", slo),
352+
func(s *terraform.State) error {
353+
rs := s.RootModule().Resources["honeycombio_slo.test"]
354+
originalID = rs.Primary.ID
355+
return nil
356+
},
357+
resource.TestCheckResourceAttr("honeycombio_slo.test", "name", "TestAcc SLO"),
358+
resource.TestCheckResourceAttr("honeycombio_slo.test", "description", "test SLO"),
359+
resource.TestCheckResourceAttr("honeycombio_slo.test", "target_percentage", "99.95"),
360+
resource.TestCheckResourceAttr("honeycombio_slo.test", "time_period", "30"),
361+
resource.TestCheckResourceAttr("honeycombio_slo.test", "datasets.#", "1"),
362+
resource.TestCheckTypeSetElemAttr("honeycombio_slo.test", "datasets.*", dataset1.Slug),
363+
),
364+
},
365+
{ // Update description - should not change ID
366+
Config: fmt.Sprintf(`
367+
resource "honeycombio_slo" "test" {
368+
name = "TestAcc SLO"
369+
description = "UPDATED integration test SLO"
370+
datasets = ["%s"]
371+
sli = "%s"
372+
target_percentage = 99.95
373+
time_period = 30
374+
375+
tags = {
376+
env = "test"
377+
team = "blue"
378+
}
379+
}`, dataset1.Slug, sliAlias.Alias),
380+
Check: resource.ComposeTestCheckFunc(
381+
testAccCheckSLOExists(t, "honeycombio_slo.test", slo),
382+
func(s *terraform.State) error {
383+
rs := s.RootModule().Resources["honeycombio_slo.test"]
384+
if rs.Primary.ID != originalID {
385+
return fmt.Errorf("expected ID to remain %s after description update, but got %s", originalID, rs.Primary.ID)
386+
}
387+
return nil
388+
},
389+
resource.TestCheckResourceAttr("honeycombio_slo.test", "description", "UPDATED integration test SLO"),
390+
resource.TestCheckResourceAttr("honeycombio_slo.test", "name", "TestAcc SLO"),
391+
resource.TestCheckResourceAttr("honeycombio_slo.test", "target_percentage", "99.95"),
392+
resource.TestCheckResourceAttr("honeycombio_slo.test", "time_period", "30"),
393+
resource.TestCheckResourceAttr("honeycombio_slo.test", "datasets.#", "1"),
394+
resource.TestCheckTypeSetElemAttr("honeycombio_slo.test", "datasets.*", dataset1.Slug),
395+
),
396+
},
397+
{ // Update datasets - should CHANGE ID (force replacement)
398+
Config: fmt.Sprintf(`
399+
resource "honeycombio_slo" "test" {
400+
name = "TestAcc SLO Update"
401+
description = "UPDATED test SLO for updates"
402+
datasets = ["%s"]
403+
sli = "%s"
404+
target_percentage = 99.99
405+
time_period = 30
406+
407+
tags = {
408+
env = "production"
409+
team = "red"
410+
owner = "devops"
411+
}
412+
}`, dataset2.Slug, sliAlias.Alias),
413+
Check: resource.ComposeTestCheckFunc(
414+
testAccCheckSLOExists(t, "honeycombio_slo.test", slo),
415+
func(s *terraform.State) error {
416+
rs := s.RootModule().Resources["honeycombio_slo.test"]
417+
if rs.Primary.ID == originalID {
418+
return fmt.Errorf("expected ID to change after datasets update, but it remained %s", originalID)
419+
}
420+
originalID = rs.Primary.ID
421+
return nil
422+
},
423+
resource.TestCheckResourceAttr("honeycombio_slo.test", "datasets.#", "1"),
424+
resource.TestCheckTypeSetElemAttr("honeycombio_slo.test", "datasets.*", dataset2.Slug),
425+
),
426+
},
427+
},
428+
})
429+
}
430+
320431
func testAccConfigSLO_basic(dataset, sliAlias string) string {
321432
return fmt.Sprintf(`
322433
resource "honeycombio_slo" "test" {

0 commit comments

Comments
 (0)