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

proposal: function checks #412

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

bschaatsbergen
Copy link
Member

@bschaatsbergen bschaatsbergen commented Jan 14, 2025

This is a simple prototype for adding new knownvalue checks, allowing a custom function to run against a state value at a specific path within a statecheck. This approach builds on a suggestion from @jar-b, following my earlier proposal to extract a value from the state by traversing a path and referencing it elsewhere. It feels like a more elegant solution, addressing the need to enable users with the ability to perform custom validations on attributes during plan and apply checks.

Here’s an example of a StringFunc that assigns the underlying value of the given path to v:

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

	resource.Test(t, resource.TestCase{
		// Provider definition omitted.
		Steps: []resource.TestStep{
			{
				// Example resource containing a string attribute named "configurable_attribute"
				Config: `resource "test_resource" "one" {}`,
				ConfigStateChecks: []statecheck.StateCheck{
					statecheck.ExpectKnownValue(
						"test_resource.one",
						tfjsonpath.New("configurable_attribute"),
						knownvalue.StringFunc(func(v string) error {
							if !strings.HasPrefix(v, "str") {
								return fmt.Errorf("value must start with 'str'")
							}
							return nil
						}),
					),
				},
			},
		},
	})
}

In this pull request you'll find a prototype for StringFunc and other functions. It's still work in progress and I'd appreciate your feedback and thoughts. Thanks!

@bschaatsbergen bschaatsbergen changed the title proposal: simplified value extraction using state checks proposal: value extraction Jan 14, 2025
This introduces a new state check that extracts an underlying attribute value by traversing the state with tfjsonpath using a specified path.
@jar-b
Copy link
Member

jar-b commented Jan 15, 2025

Just an observer comment after thinking on this a bit from the provider perspective. I wonder if this problem could be solved via a more generic known value check that accepts a user defined function. For string types, the implementation could look something like:

// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package knownvalue

import "fmt"

var _ Check = stringFunc{}

type stringFunc struct {
	checkFunc func(v string) error
}

// CheckValue determines whether the passed value is of type string, and
// returns no error from the provided check function
func (v stringFunc) CheckValue(other any) error {
	otherVal, ok := other.(string)

	if !ok {
		return fmt.Errorf("expected string value for StringFunc check, got: %T", other)
	}

	return v.checkFunc(otherVal)
}

// String returns the string representation of the value.
func (v stringFunc) String() string {
	// Validation is up the the implementer of the function, so there are no
	// string literal or regex comparers to print here
	return "stringFunc"
}

// StringFunc returns a Check for passing the string value in state
// to the provided check function
func StringFunc(fn func(v string) error) stringFunc {
	return stringFunc{
		checkFunc: fn,
	}
}

Where a test step could use the value pulled from state to do whatever custom validation is required.

                        knownvalue.StringFunc(func(s string) error {
                                // custom validation here
                                if s != "foo" {
                                        return fmt.Errorf("%s was not foo", s)
                                }
                                return nil
                        }),

I'm not sure if this is too generic for inclusion in the testing library itself. If so, this could be a custom known value check defined once in a provider, then re-used across test cases which bring their own validation functions as necessary.

@bschaatsbergen bschaatsbergen changed the title proposal: value extraction proposal: custom checks Jan 15, 2025
@bschaatsbergen bschaatsbergen changed the title proposal: custom checks proposal: custom (function) checks Jan 15, 2025
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

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

Thanks for writing this up! Regardless of which approach is taken I think having some form of "extract this thing from this artifact" would be useful. Left some comments

Copy link
Member

Choose a reason for hiding this comment

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

@jar-b @bschaatsbergen Would be interested to hear your thoughts on why use a knownvalue check over statecheck to execute this function. I have some assumptions, but it'd help me get a better idea of what you want to use it for!

Copy link
Member Author

@bschaatsbergen bschaatsbergen Jan 20, 2025

Choose a reason for hiding this comment

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

In my initial prototype, I used statecheck functions to extract and assign a resource attribute value to a variable, and while that worked fine, it required a subtest to assert the value. It didn't feel in line with the existing test-writing DX that provider developers are familiar with. Added, referencing the variable in another check or a subsequent TestStep wasn’t really an option since those steps run in different contexts. So while the approach did the job, it felt a bit limiting.

Switching to knownvalue checks felt like a better fit—as these new functions expose the underlying value directly through the function signature, making it easier for users to define their own validation logic inside the check, instead of using a subtest. The actual assertion of the value also happens within the knownvalue check itself. Plus, it keeps everything nicely self-contained within the TestCase, which aligns well with the current API design.

One thing I really like about this approach is how it allows for simple, inline validation:

knownvalue.StringFunc(func(v string) error {
	if !strings.HasPrefix(v, "str") {
		return fmt.Errorf("value must start with 'str'")
	}
	return nil
})

Would love to hear your thoughts on this too, @austinvalle!

Comment on lines +10 to +16
type stringFunc struct {
checkFunc func(v string) error
}

// CheckValue determines whether the passed value is of type string, and
// returns no error from the provided check function
func (v stringFunc) CheckValue(value any) error {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be a generic function? I'm not super versed in Go's generic functionality, so I'm not sure if you can run a type assertion with a type parameter.

Copy link
Member Author

@bschaatsbergen bschaatsbergen Jan 20, 2025

Choose a reason for hiding this comment

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

While it’s definitely possible to write a generic function for this, which would involve some type assertions and reflection, I personally prefer our approach of offering typed functions like knownvalue.Int32Exact, knownvalue.StringRegexp, etc., in our libraries. I believe that an Int32Func, for example, aligns better with the current API design, as it’s more intuitive for users who are already accustomed to working with these typed functions.

It’s an option to go the generic route, but I think sticking with typed functions here makes it easier for users to follow. Does that make sense?

@bschaatsbergen bschaatsbergen marked this pull request as ready for review January 20, 2025 14:55
@bschaatsbergen bschaatsbergen requested review from a team as code owners January 20, 2025 14:55
@bschaatsbergen bschaatsbergen changed the title proposal: custom (function) checks proposal: function checks Jan 21, 2025
Copy link

@pbortnick pbortnick left a comment

Choose a reason for hiding this comment

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

👍 Looks fine from web presence perspective. Deferring to others for content approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants