Skip to content

Commit dbaf109

Browse files
committed
Write a reconciliation function that paves over inconsistencies in API responses
This should result in the TF provider being a little more resistant to upstream field changes.
1 parent 8437dcc commit dbaf109

29 files changed

+1221
-254
lines changed

.github/workflows/check.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ jobs:
4343
- name: Test
4444
run: |
4545
echo "provider_installation { dev_overrides { \"registry.terraform.io/ably/ably\" = \"$PWD/bin\", } direct {} }" > ~/.terraformrc
46-
TF_ACC=1 go test -v -parallel 4 ./...
46+
TF_ACC=1 go test -timeout 20m -v -parallel 4 ./...
4747
env:
4848
ABLY_ACCOUNT_TOKEN: ${{ secrets.ABLY_ACCOUNT_TOKEN }}
4949
ABLY_URL: 'https://staging-control.ably-dev.net/v1'

internal/provider/ingress_rules.go

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ func GetPlanIngressRule(plan AblyIngressRule) (any, diag.Diagnostics) {
6969
// GetIngressRuleResponse maps an API rule response to the ingress rule terraform model.
7070
// Ingress rules use the same generic RuleResponse from the client, with target unmarshalled
7171
// according to the ruleType.
72-
func GetIngressRuleResponse(ablyRule *control.RuleResponse, plan *AblyIngressRule) (AblyIngressRule, diag.Diagnostics) {
72+
func GetIngressRuleResponse(ablyRule *control.RuleResponse, plan *AblyIngressRule, reading bool) (AblyIngressRule, diag.Diagnostics) {
7373
var diags diag.Diagnostics
7474
var respTarget any
7575

@@ -113,10 +113,14 @@ func GetIngressRuleResponse(ablyRule *control.RuleResponse, plan *AblyIngressRul
113113
return AblyIngressRule{}, diags
114114
}
115115

116+
rc := newReconciler(&diags)
117+
if reading {
118+
rc.forRead()
119+
}
116120
respRule := AblyIngressRule{
117-
ID: types.StringValue(ablyRule.ID),
118-
AppID: types.StringValue(ablyRule.AppID),
119-
Status: types.StringValue(ablyRule.Status),
121+
ID: rc.str("id", plan.ID, types.StringValue(ablyRule.ID), true),
122+
AppID: rc.str("app_id", plan.AppID, types.StringValue(ablyRule.AppID), false),
123+
Status: rc.str("status", plan.Status, types.StringValue(ablyRule.Status), true),
120124
Target: respTarget,
121125
}
122126

@@ -188,7 +192,7 @@ func CreateIngressRule[T any](r Rule, ctx context.Context, req resource.CreateRe
188192
return
189193
}
190194

191-
responseValues, respDiags := GetIngressRuleResponse(&rule, &plan)
195+
responseValues, respDiags := GetIngressRuleResponse(&rule, &plan, false)
192196
resp.Diagnostics.Append(respDiags...)
193197
if resp.Diagnostics.HasError() {
194198
return
@@ -232,7 +236,7 @@ func ReadIngressRule[T any](r Rule, ctx context.Context, req resource.ReadReques
232236
return
233237
}
234238

235-
responseValues, respDiags := GetIngressRuleResponse(&rule, &state)
239+
responseValues, respDiags := GetIngressRuleResponse(&rule, &state, true)
236240
resp.Diagnostics.Append(respDiags...)
237241
if resp.Diagnostics.HasError() {
238242
return
@@ -278,7 +282,7 @@ func UpdateIngressRule[T any](r Rule, ctx context.Context, req resource.UpdateRe
278282
return
279283
}
280284

281-
responseValues, respDiags := GetIngressRuleResponse(&rule, &plan)
285+
responseValues, respDiags := GetIngressRuleResponse(&rule, &plan, false)
282286
resp.Diagnostics.Append(respDiags...)
283287
if resp.Diagnostics.HasError() {
284288
return

internal/provider/provider_test.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
package provider
33

44
import (
5+
"fmt"
56
"os"
67
"testing"
78

@@ -22,3 +23,53 @@ func testAccPreCheck(t *testing.T) {
2223
t.Fatal("ABLY_ACCOUNT_TOKEN must be set for acceptance tests")
2324
}
2425
}
26+
27+
// tfProvider is the shared Terraform provider configuration block used by
28+
// minimal-config acceptance tests across resource test files.
29+
const tfProvider = `
30+
terraform {
31+
required_providers {
32+
ably = {
33+
source = "registry.terraform.io/ably/ably"
34+
}
35+
}
36+
}
37+
provider "ably" {}
38+
`
39+
40+
// minimalRuleConfig builds an HCL config with only required fields for a rule
41+
// resource. The targetHCL argument is the target block content specific to
42+
// each rule type.
43+
func minimalRuleConfig(appName, resourceType, targetHCL string) string {
44+
return fmt.Sprintf(`%s
45+
resource "ably_app" "app0" {
46+
name = %q
47+
}
48+
49+
resource %q "rule0" {
50+
app_id = ably_app.app0.id
51+
52+
source = {
53+
type = "channel.message"
54+
}
55+
56+
%s
57+
}
58+
`, tfProvider, appName, resourceType, targetHCL)
59+
}
60+
61+
// minimalIngressRuleConfig builds an HCL config with only required fields for
62+
// an ingress rule resource.
63+
func minimalIngressRuleConfig(appName, resourceType, targetHCL string) string {
64+
return fmt.Sprintf(`%s
65+
resource "ably_app" "app0" {
66+
name = %q
67+
}
68+
69+
resource %q "rule0" {
70+
app_id = ably_app.app0.id
71+
72+
%s
73+
}
74+
`, tfProvider, appName, resourceType, targetHCL)
75+
}

internal/provider/reconcile.go

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
package provider
2+
3+
import (
4+
"fmt"
5+
6+
"github.com/hashicorp/terraform-plugin-framework/diag"
7+
"github.com/hashicorp/terraform-plugin-framework/types"
8+
)
9+
10+
// reconciler accumulates diagnostics so callers can reconcile many fields
11+
// without checking errors after each one.
12+
//
13+
// When reading is true (set via forRead), every field is treated as computed
14+
// so the API response is always accepted. This is necessary because during
15+
// Read (including import), the prior state may be empty — there is no user
16+
// plan to compare against.
17+
type reconciler struct {
18+
diags *diag.Diagnostics
19+
reading bool
20+
}
21+
22+
func newReconciler(diags *diag.Diagnostics) *reconciler {
23+
return &reconciler{diags: diags}
24+
}
25+
26+
func (r *reconciler) forRead() *reconciler {
27+
r.reading = true
28+
return r
29+
}
30+
31+
func (r *reconciler) str(field string, input, output types.String, computed bool) types.String {
32+
v, err := reconcileString(field, input, output, computed || r.reading)
33+
if err != nil {
34+
r.diags.AddError("State reconciliation error", err.Error())
35+
}
36+
return v
37+
}
38+
39+
func (r *reconciler) boolean(field string, input, output types.Bool, computed bool) types.Bool {
40+
v, err := reconcileBool(field, input, output, computed || r.reading)
41+
if err != nil {
42+
r.diags.AddError("State reconciliation error", err.Error())
43+
}
44+
return v
45+
}
46+
47+
func (r *reconciler) int64val(field string, input, output types.Int64, computed bool) types.Int64 {
48+
v, err := reconcileInt64(field, input, output, computed || r.reading)
49+
if err != nil {
50+
r.diags.AddError("State reconciliation error", err.Error())
51+
}
52+
return v
53+
}
54+
55+
// Reconcile functions handle the four-way matrix of input (plan/state) vs
56+
// output (API response) emptiness:
57+
//
58+
// 1. Input non-empty, Output non-empty → return output (API is authoritative)
59+
// 2. Input non-empty, Output empty → return input (write-only / sensitive field)
60+
// 3. Input empty, Output non-empty:
61+
// - computed=true → return output (server-owned or defaulted field)
62+
// - computed=false → return error (unexpected value for optional-only field)
63+
// 4. Input empty, Output empty → return null
64+
//
65+
// "Empty" means null, unknown, or (for strings) the zero-length string "".
66+
67+
// reconcileString reconciles a plan/state string with an API response string.
68+
func reconcileString(field string, input, output types.String, computed bool) (types.String, error) {
69+
inputEmpty := input.IsNull() || input.IsUnknown() || input.ValueString() == ""
70+
outputEmpty := output.IsNull() || output.IsUnknown() || output.ValueString() == ""
71+
72+
switch {
73+
case !inputEmpty && !outputEmpty:
74+
return output, nil
75+
case !inputEmpty && outputEmpty:
76+
return input, nil
77+
case inputEmpty && !outputEmpty:
78+
if computed {
79+
return output, nil
80+
}
81+
return types.StringNull(), fmt.Errorf(
82+
"reconcile %q: API returned %q but field was not set in config and is not computed",
83+
field, output.ValueString(),
84+
)
85+
default:
86+
return types.StringNull(), nil
87+
}
88+
}
89+
90+
// reconcileBool reconciles a plan/state bool with an API response bool.
91+
func reconcileBool(field string, input, output types.Bool, computed bool) (types.Bool, error) {
92+
inputEmpty := input.IsNull() || input.IsUnknown()
93+
outputEmpty := output.IsNull() || output.IsUnknown()
94+
95+
switch {
96+
case !inputEmpty && !outputEmpty:
97+
return output, nil
98+
case !inputEmpty && outputEmpty:
99+
return input, nil
100+
case inputEmpty && !outputEmpty:
101+
if computed {
102+
return output, nil
103+
}
104+
return types.BoolNull(), fmt.Errorf(
105+
"reconcile %q: API returned %t but field was not set in config and is not computed",
106+
field, output.ValueBool(),
107+
)
108+
default:
109+
return types.BoolNull(), nil
110+
}
111+
}
112+
113+
// reconcileInt64 reconciles a plan/state int64 with an API response int64.
114+
func reconcileInt64(field string, input, output types.Int64, computed bool) (types.Int64, error) {
115+
inputEmpty := input.IsNull() || input.IsUnknown()
116+
outputEmpty := output.IsNull() || output.IsUnknown()
117+
118+
switch {
119+
case !inputEmpty && !outputEmpty:
120+
return output, nil
121+
case !inputEmpty && outputEmpty:
122+
return input, nil
123+
case inputEmpty && !outputEmpty:
124+
if computed {
125+
return output, nil
126+
}
127+
return types.Int64Null(), fmt.Errorf(
128+
"reconcile %q: API returned %d but field was not set in config and is not computed",
129+
field, output.ValueInt64(),
130+
)
131+
default:
132+
return types.Int64Null(), nil
133+
}
134+
}

0 commit comments

Comments
 (0)