-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding State Checks for Known Type and Value, and Sensitive Checks #275
Changes from 27 commits
f67b3e7
eed2a12
d4e31d3
4e3ca3a
d93aa82
598dac8
c78e3e8
1e51693
3d4acf4
01fe9a9
fb9fed8
7ff68bc
5d04859
a79aea8
f5abf73
648730a
c74a9e8
178c2c4
c542a70
518c94c
35ffc45
31f8d5e
031df21
6df33b9
15330e3
e4e96ac
a88a10d
6d8f112
04cf3c9
173d4b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: FEATURES | ||
body: 'statecheck: Introduced new `statecheck` package with interface and built-in | ||
state check functionality' | ||
time: 2024-01-11T14:21:26.261094Z | ||
custom: | ||
Issue: "275" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: FEATURES | ||
body: 'statecheck: Added `ExpectKnownValue` state check, which asserts that a given | ||
resource attribute has a defined type, and value' | ||
time: 2024-01-11T14:22:23.072321Z | ||
custom: | ||
Issue: "275" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: FEATURES | ||
body: 'statecheck: Added `ExpectKnownOutputValue` state check, which asserts that | ||
a given output value has a defined type, and value' | ||
time: 2024-01-11T14:23:14.025585Z | ||
custom: | ||
Issue: "275" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: FEATURES | ||
body: 'statecheck: Added `ExpectKnownOutputValueAtPath` plan check, which asserts | ||
that a given output value at a specified path has a defined type, and value' | ||
time: 2024-01-11T14:23:53.633255Z | ||
custom: | ||
Issue: "275" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: FEATURES | ||
body: 'statecheck: Added `ExpectSensitiveValue` built-in state check, which asserts | ||
that a given attribute has a sensitive value' | ||
time: 2024-01-11T14:25:44.598583Z | ||
custom: | ||
Issue: "275" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
kind: NOTES | ||
body: 'plancheck: Deprecated `ExpectNullOutputValue` and `ExpectNullOutputValueAtPath`. | ||
Use `ExpectKnownOutputValue` and `ExpectKnownOutputValueAtPath` with | ||
`knownvalue.NullExact` instead' | ||
time: 2024-01-22T08:26:28.053303Z | ||
custom: | ||
Issue: "275" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
// Copyright (c) HashiCorp, Inc. | ||
// SPDX-License-Identifier: MPL-2.0 | ||
|
||
package resource | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
|
||
tfjson "github.com/hashicorp/terraform-json" | ||
"github.com/mitchellh/go-testing-interface" | ||
|
||
"github.com/hashicorp/terraform-plugin-testing/statecheck" | ||
) | ||
|
||
func runStateChecks(ctx context.Context, t testing.T, state *tfjson.State, stateChecks []statecheck.StateCheck) error { | ||
t.Helper() | ||
|
||
var result []error | ||
|
||
for _, stateCheck := range stateChecks { | ||
resp := statecheck.CheckStateResponse{} | ||
stateCheck.CheckState(ctx, statecheck.CheckStateRequest{State: state}, &resp) | ||
|
||
result = append(result, resp.Error) | ||
} | ||
|
||
return errors.Join(result...) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
// Copyright (c) HashiCorp, Inc. | ||
// SPDX-License-Identifier: MPL-2.0 | ||
|
||
package resource | ||
|
||
import ( | ||
"context" | ||
|
||
"github.com/hashicorp/terraform-plugin-testing/statecheck" | ||
) | ||
|
||
var _ statecheck.StateCheck = &stateCheckSpy{} | ||
|
||
type stateCheckSpy struct { | ||
err error | ||
called bool | ||
} | ||
|
||
func (s *stateCheckSpy) CheckState(ctx context.Context, req statecheck.CheckStateRequest, resp *statecheck.CheckStateResponse) { | ||
s.called = true | ||
resp.Error = s.err | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ import ( | |
|
||
"github.com/hashicorp/terraform-plugin-testing/config" | ||
"github.com/hashicorp/terraform-plugin-testing/plancheck" | ||
"github.com/hashicorp/terraform-plugin-testing/statecheck" | ||
"github.com/hashicorp/terraform-plugin-testing/terraform" | ||
"github.com/hashicorp/terraform-plugin-testing/tfversion" | ||
|
||
|
@@ -590,6 +591,13 @@ type TestStep struct { | |
// [plancheck]: https://pkg.go.dev/github.com/hashicorp/terraform-plugin-testing/plancheck | ||
RefreshPlanChecks RefreshPlanChecks | ||
|
||
// ConfigStateChecks allow assertions to be made against the state file at different points of a Config (apply) test using a state check. | ||
// Custom state checks can be created by implementing the [StateCheck] interface, or by using a StateCheck implementation from the provided [statecheck] package | ||
// | ||
// [StateCheck]: https://pkg.go.dev/github.com/hashicorp/terraform-plugin-testing/statecheck#StateCheck | ||
// [statecheck]: https://pkg.go.dev/github.com/hashicorp/terraform-plugin-testing/statecheck | ||
ConfigStateChecks ConfigStateChecks | ||
bendbennett marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// PlanOnly can be set to only run `plan` with this configuration, and not | ||
// actually apply it. This is useful for ensuring config changes result in | ||
// no-op plans | ||
|
@@ -795,6 +803,10 @@ type RefreshPlanChecks struct { | |
PostRefresh []plancheck.PlanCheck | ||
} | ||
|
||
// ConfigStateChecks runs all state checks in the slice. This occurs after the apply and refresh of a Config test are run. | ||
// All errors by state checks in this slice are aggregated, reported, and will result in a test failure. | ||
type ConfigStateChecks []statecheck.StateCheck | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Open question: do we need this additional exported type? I'm guessing this was for a little bit of consistency with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're assumption is correct, the exported |
||
|
||
// ParallelTest performs an acceptance test on a resource, allowing concurrency | ||
// with other ParallelTest. The number of concurrent tests is controlled by the | ||
// "go test" command -parallel flag. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -313,6 +313,26 @@ func testStepNewConfig(ctx context.Context, t testing.T, c TestCase, wd *plugint | |
} | ||
} | ||
|
||
// Run post-apply, post-refresh state checks | ||
if len(step.ConfigStateChecks) > 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Historically we have ran state checks immediately after apply and before the additional plan checks (e.g. line 181 area) -- do we want to also do that here for consistency/ease of migration? In reality, the timing of the checks will determine which errors developers might see first: whether it be unexpectedly non-empty plans after apply or whether their state assertions are incorrect. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've moved the execution of the |
||
var state *tfjson.State | ||
|
||
err = runProviderCommand(ctx, t, func() error { | ||
var err error | ||
state, err = wd.State(ctx) | ||
return err | ||
}, wd, providers) | ||
|
||
if err != nil { | ||
return fmt.Errorf("Error retrieving post-apply, post-refresh state: %w", err) | ||
} | ||
|
||
err = runStateChecks(ctx, t, state, step.ConfigStateChecks) | ||
if err != nil { | ||
return fmt.Errorf("Post-apply refresh state check(s) failed:\n%w", err) | ||
} | ||
} | ||
|
||
// check if plan is empty | ||
if !planIsEmpty(plan, helper.TerraformVersion()) && !step.ExpectNonEmptyPlan { | ||
var stdout string | ||
|
austinvalle marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
// Copyright (c) HashiCorp, Inc. | ||
// SPDX-License-Identifier: MPL-2.0 | ||
|
||
package knownvalue | ||
|
||
import ( | ||
"fmt" | ||
) | ||
|
||
var _ Check = nullExact{} | ||
|
||
type nullExact struct{} | ||
|
||
// CheckValue determines whether the passed value is nil. | ||
func (v nullExact) CheckValue(other any) error { | ||
if other != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I vaguely remember checking an And delve shows it slightly different, but still should be considered There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This article explains the problem a bit, may have to dip into some reflection if we need to cover nil cases like maps and slices. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for raising this. I've examined the values returned when using the With a schema that looks as follows: func (e *exampleResource) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) {
resp.Schema = schema.Schema{
Attributes: map[string]schema.Attribute{
/* ... */
"list_attribute": schema.ListAttribute{
Optional: true,
/* ... */
},
"map_attribute": schema.MapAttribute{
Optional: true,
/* ... */
},
"object_attribute": schema.ObjectAttribute{
Optional: true,
AttributeTypes: map[string]attr.Type{
/* ... */
},
},
"set_attribute": schema.SetAttribute{
Optional: true,
/* ... */
},
"list_nested_attribute": schema.ListNestedAttribute{
Optional: true,
NestedObject: schema.NestedAttributeObject{
Attributes: map[string]schema.Attribute{
/* ... */
},
},
},
"map_nested_attribute": schema.MapNestedAttribute{
Optional: true,
NestedObject: schema.NestedAttributeObject{
Attributes: map[string]schema.Attribute{
/* ... */
},
},
},
"set_nested_attribute": schema.SetNestedAttribute{
Optional: true,
NestedObject: schema.NestedAttributeObject{
Attributes: map[string]schema.Attribute{
/* ... */
},
},
},
"single_nested_attribute": schema.SingleNestedAttribute{
Optional: true,
Attributes: map[string]schema.Attribute{
/* ... */
},
},
},
Blocks: map[string]schema.Block{
"list_nested_block": schema.ListNestedBlock{
NestedObject: schema.NestedBlockObject{
Attributes: map[string]schema.Attribute{
/* ... */
},
Blocks: map[string]schema.Block{
"list_nested_nested_block": schema.ListNestedBlock{
NestedObject: schema.NestedBlockObject{
Attributes: map[string]schema.Attribute{
/* ... */
},
},
},
},
},
},
"set_nested_block": schema.SetNestedBlock{
NestedObject: schema.NestedBlockObject{
Attributes: map[string]schema.Attribute{
/* ... */
},
Blocks: map[string]schema.Block{
"set_nested_nested_block": schema.SetNestedBlock{
NestedObject: schema.NestedBlockObject{
Attributes: map[string]schema.Attribute{
/* ... */
},
},
},
},
},
},
"single_nested_block": schema.SingleNestedBlock{
Attributes: map[string]schema.Attribute{
/* ... */
},
},
},
}
} The plan output looks as follows: {
"format_version": "1.2",
"terraform_version": "1.7.0",
"planned_values": {
"root_module": {
"resources": [
{
"address": "example_resource.example",
"mode": "managed",
"type": "example_resource",
"name": "example",
"provider_name": "registry.terraform.io/bendbennett/playground",
"schema_version": 0,
"values": {
"list_attribute": null,
"list_nested_attribute": null,
"list_nested_block": [],
"map_attribute": null,
"map_nested_attribute": null,
"object_attribute": null,
"set_attribute": null,
"set_nested_attribute": null,
"set_nested_block": [],
"single_nested_attribute": null,
"single_nested_block": null,
},
"sensitive_values": {
"list_nested_block": [],
"set_nested_block": []
}
}
]
}
},
"resource_changes": [
{
"address": "example_resource.example",
"mode": "managed",
"type": "example_resource",
"name": "example",
"provider_name": "registry.terraform.io/bendbennett/playground",
"change": {
"actions": [
"create"
],
"before": null,
"after": {
"list_attribute": null,
"list_nested_attribute": null,
"list_nested_block": [],
"map_attribute": null,
"map_nested_attribute": null,
"object_attribute": null,
"set_attribute": null,
"set_nested_attribute": null,
"set_nested_block": [],
"single_nested_attribute": null,
"single_nested_block": null,
},
"after_unknown": {
"id": true,
"list_nested_block": [],
"set_nested_block": []
},
"before_sensitive": false,
"after_sensitive": {
"list_nested_block": [],
"set_nested_block": []
}
}
}
],
"configuration": {
"provider_config": {
"example": {
"name": "example",
"full_name": "registry.terraform.io/bendbennett/playground"
},
"playground": {
"name": "playground",
"full_name": "registry.terraform.io/bendbennett/playground"
}
},
"root_module": {
"resources": [
{
"address": "example_resource.example",
"mode": "managed",
"type": "example_resource",
"name": "example",
"provider_config_key": "example",
"schema_version": 0
}
]
}
},
"timestamp": "2024-01-22T10:12:40Z",
"errored": false
} The state looks as follows: {
"version": 4,
"terraform_version": "1.7.0",
"serial": 1,
"lineage": "2c8dadf4-11dd-22b9-c419-5d07aa808890",
"outputs": {},
"resources": [
{
"mode": "managed",
"type": "example_resource",
"name": "example",
"provider": "provider[\"registry.terraform.io/bendbennett/playground\"]",
"instances": [
{
"schema_version": 0,
"attributes": {
"bool_attribute": null,
"float64_attribute": null,
"id": "example-id",
"int64_attribute": null,
"list_attribute": null,
"list_nested_attribute": null,
"list_nested_block": [],
"map_attribute": null,
"map_nested_attribute": null,
"number_attribute": null,
"object_attribute": null,
"set_attribute": null,
"set_nested_attribute": null,
"set_nested_block": [],
"single_nested_attribute": null,
"single_nested_block": null,
"string_attribute": null
},
"sensitive_attributes": []
}
]
}
],
"check_results": null
} Debugging during test execution shows the following: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've added some additional test coverage to the following:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Today I learned! I think we have historically avoided these sorts of issues by not performing (re-)assignment like that article shows. Hopefully we can rely on the static analysis tooling to catch the situation if for some reason it does get introduced and the unit testing does not catch it, because I'm a proponent of simpler == better where possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding the tests, good to know it shouldn't be a problem for us 🙂
Agreed 👍🏻 |
||
return fmt.Errorf("expected value nil for NullExact check, got: %T", other) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// String returns the string representation of null. | ||
func (v nullExact) String() string { | ||
return "null" | ||
} | ||
|
||
// NullExact returns a Check for asserting equality nil | ||
// and the value passed to the CheckValue method. | ||
func NullExact() nullExact { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bikeshed (I'm so sorry): Are there other checks that could be run against a null value? Maybe this can be just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Agree with the suggested naming change here. I've made the following changes:
The docs have been updated in accordance with these changes. |
||
return nullExact{} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No more
resource.ComposeAggregateTestCheckFunc
vsresource.ComposeTestCheckFunc
debates!