Skip to content
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 plan checks for ExpectKnownValue, ExpectKnownOutputValue, and ExpectKnownOutputValueAtPath #248

Merged
merged 39 commits into from
Jan 15, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
34cee4a
Add KnownValue interface and types (#243)
bendbennett Dec 15, 2023
6915f0e
Add ExpectKnownValue plan check (#243)
bendbennett Dec 15, 2023
f503594
Handling different permutations for equality checking of interface ty…
bendbennett Dec 15, 2023
4f8eed9
Adding tests for missing resource, and attribute value null (#243)
bendbennett Dec 15, 2023
a1f35f6
Adding plan checks for known output value and known output value at p…
bendbennett Dec 18, 2023
853a3e4
Adding documentation (#243)
bendbennett Dec 18, 2023
f88c44b
Merge branch 'main' into bendbennett/issues-243
bendbennett Dec 18, 2023
0ccf758
Adding changelog entries (#243)
bendbennett Dec 18, 2023
f161203
Adding TerraformVersionChecks (#243)
bendbennett Dec 18, 2023
e13bdaa
Modifying to handle numerical values returned as json.Number for tfjs…
bendbennett Dec 20, 2023
916674e
Merge remote-tracking branch 'origin/main' into bendbennett/issues-243
bendbennett Dec 20, 2023
450339d
Renaming known value constructors (#243)
bendbennett Jan 2, 2024
31cbf09
Refactoring to Check interface (#243)
bendbennett Jan 3, 2024
73a5300
Merge remote-tracking branch 'origin/main' into bendbennett/issues-243
bendbennett Jan 3, 2024
3f14259
Linting (#243)
bendbennett Jan 3, 2024
5e61524
Modifying known value check error messages and tests (#243)
bendbennett Jan 3, 2024
41eb4de
Updating tests for ExpectKnownValue, ExpectKnownOutputValue and Expec…
bendbennett Jan 3, 2024
94bdfa7
Adding changelog entry to note the switch to using json.Number for nu…
bendbennett Jan 4, 2024
f09f7e9
Remove reference to state checks (#243)
bendbennett Jan 4, 2024
7c5c177
Moving concepts under title and removing reference to Framework types…
bendbennett Jan 4, 2024
1638808
Updating Go doc comments to clarify usage of partial equality and rem…
bendbennett Jan 4, 2024
213b81a
Modifying known-values.mdx page description (#243)
bendbennett Jan 4, 2024
fa8f125
Restructuring and updating references to knownvalue.Check (#243)
bendbennett Jan 4, 2024
91417e1
Adding individual docs pages for each type of known value check (#243)
bendbennett Jan 4, 2024
463c0e3
Removing references to num elements (#243)
bendbennett Jan 4, 2024
a661490
Removing references to state (#243)
bendbennett Jan 4, 2024
6b626f4
Adding docs page for custom known value checks (#243)
bendbennett Jan 4, 2024
ad071ae
Fixing error message (#243)
bendbennett Jan 5, 2024
efb0b98
Refactoring to accomodate custom known value checks in ExpectKnownVal…
bendbennett Jan 5, 2024
9e5cde1
Apply suggestions from code review
bendbennett Jan 11, 2024
d14429e
Unexporting types that implement known value check (#266)
bendbennett Jan 11, 2024
f1b9656
Document usage of 512-bit precision in the number known value check (…
bendbennett Jan 11, 2024
ed9d236
Adding attribute or output path to error message (#266)
bendbennett Jan 11, 2024
9bdf445
Replacing alias in example code (#266)
bendbennett Jan 11, 2024
0849854
Rename file (#266)
bendbennett Jan 11, 2024
4f283cf
Merge remote-tracking branch 'refs/remotes/origin/bendbennett/issues-…
bendbennett Jan 11, 2024
edcddb6
Renamed list, map, and set element length checks to <List|Map|Set>Siz…
bendbennett Jan 15, 2024
5f4c952
Removing ObjectAttributesExact (#243)
bendbennett Jan 15, 2024
aab63ef
Renaming known value check types (#243)
bendbennett Jan 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/FEATURES-20231218-114539.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: FEATURES
body: 'plancheck: Added `ExpectKnownValue` plan check, which asserts that a given
resource attribute has a defined type, and value'
time: 2023-12-18T11:45:39.181954Z
custom:
Issue: "248"
6 changes: 6 additions & 0 deletions .changes/unreleased/FEATURES-20231218-114553.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: FEATURES
body: 'plancheck: Added `ExpectKnownOutputValue` plan check, which asserts that a
given output value has a defined type, and value'
time: 2023-12-18T11:45:53.272412Z
custom:
Issue: "248"
6 changes: 6 additions & 0 deletions .changes/unreleased/FEATURES-20231218-114611.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: FEATURES
body: 'plancheck: Added `ExpectKnownOutputValueAtPath` plan check, which asserts that
a given output value at a specified path has a defined type, and value'
time: 2023-12-18T11:46:11.58053Z
custom:
Issue: "248"
6 changes: 6 additions & 0 deletions .changes/unreleased/FEATURES-20231218-114739.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: FEATURES
body: 'knownvalue: Introduced new `knownvalue` package which contains types for working
with plan checks and state checks'
time: 2023-12-18T11:47:39.059813Z
custom:
Issue: "248"
43 changes: 43 additions & 0 deletions knownvalue/bool.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package knownvalue

import "strconv"

var _ KnownValue = BoolValue{}

// BoolValue is a KnownValue for asserting equality between the value
// supplied to NewBoolValue and the value passed to the Equal method.
type BoolValue struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions:

  1. Should we leave these Check implementations as unexported except for the functions (e.g. boolValue here)? It'll clean up the public Go package documentation nicely and I'm not sure if there is anything that should need to reference/extend the implementation directly since its value field is unexported.
  2. If doing that, maybe these names can then match the exported functions? e.g. boolValueExact for some additional clarity while reading the code

We can always export additional things in the future if we need! 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for reducing the amount of exported types 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

The known value check types have all be modified so that they are camel-cased versions of the exported functions, for example:

  • boolValueExact => BoolValueExact
  • listElementsExact => ListElementsExact`
  • listValueExact => ListValueExact
  • listValuePartial => ListValuePartial (note that the constructor for the know value partial types has changed from <List|Map|Object|Set>ValuePartialMatch to <List|Map|Object|Set>ValuePartial)

value bool
}

// Equal determines whether the passed value is of type bool, and
// contains a matching bool value.
func (v BoolValue) Equal(other any) bool {
otherVal, ok := other.(bool)

if !ok {
return false
}

if otherVal != v.value {
return false
}

return true
}

// String returns the string representation of the bool value.
func (v BoolValue) String() string {
return strconv.FormatBool(v.value)
}

// NewBoolValue returns a KnownValue for asserting equality between the
// supplied bool and the value passed to the Equal method.
func NewBoolValue(value bool) BoolValue {
bendbennett marked this conversation as resolved.
Show resolved Hide resolved
return BoolValue{
value: value,
}
}
57 changes: 57 additions & 0 deletions knownvalue/bool_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package knownvalue_test

import (
"testing"

"github.com/google/go-cmp/cmp"

"github.com/hashicorp/terraform-plugin-testing/knownvalue"
)

func TestBoolValue_Equal(t *testing.T) {
t.Parallel()

testCases := map[string]struct {
other any
expected bool
}{
"nil": {},
"wrong-type": {
other: 1.23,
},
"not-equal": {
other: false,
},
"equal": {
other: true,
expected: true,
},
}

for name, testCase := range testCases {
name, testCase := name, testCase

t.Run(name, func(t *testing.T) {
t.Parallel()

got := knownvalue.NewBoolValue(true).Equal(testCase.other)
bendbennett marked this conversation as resolved.
Show resolved Hide resolved

if diff := cmp.Diff(got, testCase.expected); diff != "" {
t.Errorf("unexpected difference: %s", diff)
}
})
}
}

func TestBoolValue_String(t *testing.T) {
t.Parallel()

got := knownvalue.NewBoolValue(true).String()

if diff := cmp.Diff(got, "true"); diff != "" {
t.Errorf("unexpected difference: %s", diff)
}
}
5 changes: 5 additions & 0 deletions knownvalue/doc.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

// Package knownvalue contains the known value interface, and types implementing the known value interface.
package knownvalue
45 changes: 45 additions & 0 deletions knownvalue/float64.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package knownvalue

import (
"strconv"
)

var _ KnownValue = Float64Value{}

// Float64Value is a KnownValue for asserting equality between the value
// supplied to NewFloat64Value and the value passed to the Equal method.
type Float64Value struct {
value float64
}

// Equal determines whether the passed value is of type float64, and
// contains a matching float64 value.
func (v Float64Value) Equal(other any) bool {
otherVal, ok := other.(float64)

if !ok {
return false
}

if otherVal != v.value {
return false
}

return true
}

// String returns the string representation of the float64 value.
func (v Float64Value) String() string {
return strconv.FormatFloat(v.value, 'f', -1, 64)
}

// NewFloat64Value returns a KnownValue for asserting equality between the
// supplied float64 and the value passed to the Equal method.
func NewFloat64Value(value float64) Float64Value {
return Float64Value{
value: value,
}
}
57 changes: 57 additions & 0 deletions knownvalue/float64_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package knownvalue_test

import (
"testing"

"github.com/google/go-cmp/cmp"

"github.com/hashicorp/terraform-plugin-testing/knownvalue"
)

func TestFloat64Value_Equal(t *testing.T) {
t.Parallel()

testCases := map[string]struct {
other any
expected bool
}{
"nil": {},
"wrong-type": {
other: 123,
},
"not-equal": {
other: false,
},
"equal": {
other: 1.23,
expected: true,
},
}

for name, testCase := range testCases {
name, testCase := name, testCase

t.Run(name, func(t *testing.T) {
t.Parallel()

got := knownvalue.NewFloat64Value(1.23).Equal(testCase.other)

if diff := cmp.Diff(got, testCase.expected); diff != "" {
t.Errorf("unexpected difference: %s", diff)
}
})
}
}

func TestFloat64Value_String(t *testing.T) {
t.Parallel()

got := knownvalue.NewFloat64Value(1.234567890123e+09).String()

if diff := cmp.Diff(got, "1234567890.123"); diff != "" {
t.Errorf("unexpected difference: %s", diff)
}
}
45 changes: 45 additions & 0 deletions knownvalue/int64.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package knownvalue

import (
"strconv"
)

var _ KnownValue = Int64Value{}

// Int64Value is a KnownValue for asserting equality between the value
// supplied to NewInt64Value and the value passed to the Equal method.
type Int64Value struct {
value int64
}

// Equal determines whether the passed value is of type int64, and
// contains a matching int64 value.
func (v Int64Value) Equal(other any) bool {
otherVal, ok := other.(int64)

if !ok {
return false
}

if otherVal != v.value {
return false
}

return true
}

// String returns the string representation of the int64 value.
func (v Int64Value) String() string {
return strconv.FormatInt(v.value, 10)
}

// NewInt64Value returns a KnownValue for asserting equality between the
// supplied int64 and the value passed to the Equal method.
func NewInt64Value(value int64) Int64Value {
return Int64Value{
value: value,
}
}
57 changes: 57 additions & 0 deletions knownvalue/int64_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package knownvalue_test

import (
"testing"

"github.com/google/go-cmp/cmp"

"github.com/hashicorp/terraform-plugin-testing/knownvalue"
)

func TestInt64Value_Equal(t *testing.T) {
t.Parallel()

testCases := map[string]struct {
other any
expected bool
}{
"nil": {},
"wrong-type": {
other: 1.23,
},
"not-equal": {
other: false,
},
"equal": {
other: int64(123),
expected: true,
},
}

for name, testCase := range testCases {
name, testCase := name, testCase

t.Run(name, func(t *testing.T) {
t.Parallel()

got := knownvalue.NewInt64Value(123).Equal(testCase.other)

if diff := cmp.Diff(got, testCase.expected); diff != "" {
t.Errorf("unexpected difference: %s", diff)
}
})
}
}

func TestInt64Value_String(t *testing.T) {
t.Parallel()

got := knownvalue.NewInt64Value(1234567890123).String()

if diff := cmp.Diff(got, "1234567890123"); diff != "" {
t.Errorf("unexpected difference: %s", diff)
}
}
10 changes: 10 additions & 0 deletions knownvalue/known_value.go
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit 🦸🏻 - Should this file be named check.go now?

Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package knownvalue

// KnownValue defines an interface that is implemented to determine equality on the basis of type and value.
type KnownValue interface {
bendbennett marked this conversation as resolved.
Show resolved Hide resolved
// Equal should perform equality testing.
Equal(other any) bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open question: Do we want to call this "check", "match", or another name? While exact equality is one use case with these, there are other use cases as well, e.g.

  • Regular expressions for strings
  • Minimum/maximum for numbers
  • Partial matching for collections or objects
  • Returning true for any value

I guess I'm just trying to ensure folks aren't confused or turned away for the not-exactly-equal use cases.

Seeing everything written out now, I almost wonder if this interface should be something like:

type Check interface {
  CheckValue(value any) error

  String() string
}

There's at least two benefits I see here beyond the potentially clearer naming:

  • They can set their own error messaging.
  • They can raise implementation errors rather than potentially hiding those types of errors based on a specific boolean answer.
  • Provider developers can implement their own implementations without needing to reimplement something like ExpectKnownValue

I hesitate to say going down the fully-type-named interfaces, e.g.

type BoolCheck interface {
  CheckBool(value bool) error

  String() string
}

Unless we'd consider passing json.Number directly for number checks. We could still have nice "int64", etc implementations even if we did that though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of returning an error and the additional flexibility that this yields.

In terms of the fully-type-named interfaces, I'm wondering about the internals. For instance, implementation of CheckBool() would be straightforward:

func (v BoolValue) CheckBool(other bool) error {
	if other != v.value {
		return fmt.Errorf("%t does not equal %t", other, v.value)
	}

	return nil
}

With the call to CheckBool() from expectKnownValue.CheckPlan() looking something like the following:

	switch reflect.TypeOf(result).Kind() {
	case reflect.Bool:
		v, ok := e.knownValue.(knownvalue.BoolValue)

		if !ok {
			resp.Error = fmt.Errorf("wrong type: attribute value is bool, known value type is %T", e.knownValue)

			return
		}

		err := v.CheckBool(result.(bool))

		if err != nil {
			resp.Error = fmt.Errorf("attribute value: %v does not equal expected value: %s", result, v)

			return
		}

However, the internals for all of the KnownValue types that deal with maps, or slices, would, I believe, require some internal type assertion to determine whether the fully-type-named interface methods could be called. For example:

func (v ObjectValue) CheckObject(other map[string]any) error {
	if len(other) != len(v.value) {
		return fmt.Errorf("%v does not equal %v", other, v.value)
	}

	for k, v := range v.value {
		otherItem, ok := other[k]

		if !ok {
			return fmt.Errorf("%s is missing from %v", k, other)
		}

		switch reflect.TypeOf(otherItem).Kind() {
		case reflect.Bool:
			boolCheck, ok := v.(BoolValue)

			if !ok {
				return fmt.Errorf("wrong type: %T, known value type is %T", otherItem, v)
			}

			if err := boolCheck.CheckBool(otherItem.(bool)); err != nil {
				return fmt.Errorf("%v does not equal %v", otherItem, v)
			} 
		// Reflection logic for reflect.Map, reflect.Slice, reflect.String
		default:
			return fmt.Errorf("unrecognised type: %T, known value type is %T", otherItem, v)
		}
	}

	return nil
}

I'll go ahead and implement the Check interface, and perhaps we can go from there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that situation, yes, I would expect the collection/object checks to need to figure out what their element or per-object-attribute check type is. Another option would be to not support that functionality and leave it up to developer implementations to figure out what to do with making more generic collection/object checks that support elements/object attributes, but I think there's a lot of value in being able to offer that out of the box.

Not sure if it will make the logic easier/harder, but another way of doing this is performing a type switch on the v.value (presumably the per-object-attribute checks in the given example code) and raising an error if the otherItem doesn't conform to the given check type. This can help make sure the error messaging is tailored for the what the developer put into their object check definition rather than tailored for the data returned from Terraform, which without additional context can be harder to reason about in my opinion. e.g.

// note also including the object attribute name (k from example) and not assuming
// it was an equality check error in error messaging (the checks can say so
// themselves, amongst other possible errors :) )
switch check := v.(type) {
case BoolValue:
  otherValue, ok := otherItem.(bool)

  if !ok {
    return fmt.Errorf("%s object attribute: expected bool value for BoolValue check, got: %T", k, otherItem)
  }

  if err := check.CheckBool(otherValue); err != nil {
    return fmt.Errorf("%s object attribute: %s", k, err)
  }
// ...
default:
  
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess a potential wrinkle here is, what if there is a dynamically typed value is being checked? Hmm. I would normally presume the value should be statically typed based on the given Terraform configuration. If not, we would be fine as long as there is some sort of escape hatch for a "I don't know the value type and I'll do all the work to figure it out" type of check, e.g. CheckDynamic(value any) error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've refactored to use the Check interface:

type Check interface {
  CheckValue(value any) error

  String() string
}

Before proceeding down the route of using fully-type-named interface, thought it might be worth considering how the ExpectKnownValue, ExpectKnownOutputValue, and ExpectKnownOutputValueAtPath would handle these fully-type-named interfaces.

Currently, taking ExpectKnownValue for instance, the type and constructor look as follows:

var _ PlanCheck = expectKnownValue{}

type expectKnownValue struct {
	resourceAddress string
	attributePath   tfjsonpath.Path
	knownValue      knownvalue.Check
}

func (e expectKnownValue) CheckPlan(ctx context.Context, req CheckPlanRequest, resp *CheckPlanResponse) {
	/* ... */
}

func ExpectKnownValue(resourceAddress string, attributePath tfjsonpath.Path, knownValue knownvalue.Check) PlanCheck {
	return expectKnownValue{
		resourceAddress: resourceAddress,
		attributePath:   attributePath,
		knownValue:      knownValue,
	}
}

As an example, BoolValue currently implements the Check interface:

var _ Check = BoolValue{}

type BoolValue struct {
	value bool
}

func (v BoolValue) CheckValue(other any) error {
	/* ... */
}

/* ... */

func BoolValueExact(value bool) BoolValue {
	return BoolValue{
		value: value,
	}
}

If we refactor BoolValue to implement the BoolCheck interface:

type BoolCheck interface {
	CheckBool(value bool) error

	String() string
}

var _ BoolCheck = BoolValue{}

type BoolValue struct {
	value bool
}

func (v BoolValue) CheckBool(value bool) error {
	/* ... */
}

/* ... */

func BoolValueExact(value bool) BoolValue {
	return BoolValue{
		value: value,
	}
}

Using fully-type-named interfaces will require that we reconsider how the constructor and struct field holding the checks will be handled for ExpectKnownValue, ExpectKnownOutputValue, and ExpectKnownOutputValueAtPath, as we'll no longer be able to construct an ExpectKnownValue plan check, for instance, using the following:

plancheck.ExpectKnownValue(
	"test_resource.one",
	tfjsonpath.New("bool_attribute"),
	knownvalue.BoolValueExact(true),
),

}
Loading
Loading