Implementing expect unknown and null output value plan checks #220

bendbennett · 2023-11-17T16:45:12Z

Closes: #219

SBGoods

Overall, this looks great! I've added minor comments mostly around documentation.

More generally, I think we should come up with common language to differentiate between attribute state and attribute output/value. Especially with expectUnknownValue() and expectUnknownOutputValue() where the language is a bit too similar. Maybe we can describe expectUnknownValue() and expectSensitiveValue() as "asserting that an attribute is being marked as unknown/sensitive."

plancheck/expect_null_output_value.go

SBGoods · 2023-11-20T18:16:03Z

tfjsonpath/path.go

 			}

 		case SliceStep:
+			steps = append(steps, fmt.Sprint(s))


Is there a use case for using fmt.Sprint(s) instead of string(s) here?

fmt.Sprint(s) is used as golangci-lint raises the following issue if string(s) is used:

Error: stringintconv: conversion from SliceStep (int) to string yields a string of one rune, not a string of digits (did you mean fmt.Sprint(x)?) (govet)

SBGoods · 2023-11-20T18:29:05Z

plancheck/expect_unknown_output_value.go

+	}
+}
+
+// OutputValueParams is used during the creation of a plan check for output values, and specifies


Is it better to move this information to the website documentation since it's directly exposed to provider developers?

We generally prefer to document terraform-plugin-* functionality that is exposed to developers via both Go package documentation and website documentation. The Go package documentation shows up on https://pkg.go.dev/github.com/hashicorp/terraform-plugin-testing as well as code editor integrations, such as the Go extension for VS Code, meaning that we can reduce developers need to context switch to a web browser while writing code.

I've added the documentation for OutputValueParams to the website docs. It now appears in both the website docs and in the Go package docs.

bflad

In general this is getting there -- just some larger things to potentially discuss/handle before getting into smaller implementation details.

bflad · 2023-11-20T18:53:31Z

plancheck/expect_unknown_output_value.go

+	}
+}
+
+// OutputValueParams is used during the creation of a plan check for output values, and specifies


We generally prefer to document terraform-plugin-* functionality that is exposed to developers via both Go package documentation and website documentation. The Go package documentation shows up on https://pkg.go.dev/github.com/hashicorp/terraform-plugin-testing as well as code editor integrations, such as the Go extension for VS Code, meaning that we can reduce developers need to context switch to a web browser while writing code.

bflad · 2023-11-20T18:57:35Z

plancheck/expect_unknown_output_value.go

+type OutputValueParams struct {
+	OutputAddress string
+	AttributePath tfjsonpath.Path
+}


👋 I think it might be good to discuss whether a separate struct is necessary or if we should expose separate functions instead (one without path, one with path). At the very least we should likely call the path information Path or ValuePath -- attribute paths don't exist within outputs since an output is a value. That value may have came from a resource with attributes, but that's not guaranteed nor the only valid usage for complex output values.

I've updated to ValuePath for now, but let's discuss potentially splitting into separate functions.

Following out-of-band discussion, the Expect<Null|Unknown>OutputValue funcs have been split into:

Expect<Null|Unknown>OutputValue

Expect<Null|Unknown>OutputValueAtPath

bflad · 2023-11-20T19:01:50Z

website/docs/plugin/testing/acceptance-tests/plan-checks.mdx

-| [`plancheck.ExpectResourceAction(address, operation)`](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-testing/plancheck#ExpectResourceAction) | Asserts the given resource has the specified operation for apply.            |
-| [`plancheck.ExpectUnknownValue(address, path)`](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-testing/plancheck#ExpectUnknownValue)          | Asserts the specified attribute at the given resource has an unknown value.  |
-| [`plancheck.ExpectSensitiveValue(address, path)`](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-testing/plancheck#ExpectSensitiveValue)      | Asserts the specified attribute at the given resource has a sensitive value. |
+| Check                                                                                                                                                      | Description                                                                  |


It might be good to split up this table (and maybe the page) to be resource attribute checks versus output value checks to reduce potential confusion.

Have split the page into Plan Checks - Overview, Resource, Output, and Custom.

The docs for ExpectEmptyPlan and ExpectNonEmptyPlan currently appear on the Resource page. Perhaps these should be on a separate page or on the Overview page?

To answer your question directly -- I think choosing the atypical option of documenting those on the Overview page is okay here since they should theoretically apply to the whole plan. ExpectEmptyPlan/ExpectNonEmptyPlan currently only check resource changes, not output changes, at the moment though. I'm guessing that most developers would also expect them to also account for other/all plan changes given the naming, so I'm going to file a bug report to adjust the existing implementation. I think if developers need more granular "only no resource changes" then we can introduce general resource vs output (non-)empty plan checks when requested. 👍

Also ❤️ for thinking ahead to just split up the pages.

Have added a General Plan Checks section to the Overview page to include plan checks for ExpectEmptyPlan/ExpectNonEmptyPlan.

Co-authored-by: Selena Goods <[email protected]>

bendbennett · 2023-11-21T09:03:07Z

More generally, I think we should come up with common language to differentiate between attribute state and attribute output/value. Especially with expectUnknownValue() and expectUnknownOutputValue() where the language is a bit too similar. Maybe we can describe expectUnknownValue() and expectSensitiveValue() as "asserting that an attribute is being marked as unknown/sensitive."

I've modified the wording on ExpectUnknownOutputValue, and ExpectNullOutputValue to emphasise the difference. For instance:

ExpectUnknownOutputValue returns a plan check that asserts that the specified output has an unknown value.

Do you think this provides enough distinction/clarity?

…Unknown>OutputValue and Expect<Null|Unknown>OutputValueAtPath (#219)

bflad

Overall, this is wonderful. Looks good to me 🚀

bflad · 2023-11-22T12:52:22Z

plancheck/expect_unknown_output_value_at_path_test.go

+		},
+		Steps: []r.TestStep{
+			{
+				Config: `resource "time_static" "one" {}


💡 If we want, we can skip using a real provider and either implement code-based provider or maybe preferably use the terraform_data resource (with requisite Terraform 1.4+ version checking) as its output attribute is automatically an unknown value of input. I personally think its okay to use terraform_data and skip the test on older Terraform versions just to simplify and speed up this testing. If there happened to be an earlier Terraform version bug with its plan output, it would not be fixed at this point.

Well, this is fun. We validate things a little too well currently if you fully omit referencing some provider in the test case/step configuration.

Test validation error: TestStep 1/2 validation error: Providers must be specified at the TestCase level, or in all TestStep, or in TestStep.ConfigDirectory or TestStep.ConfigFile

I'll create a separate issue to see if we can make it possible to create testing with just the built-in terraform provider.

Ah ha -- found a workaround based on a special provider discovery rule:

ExternalProviders: map[string]resource.ExternalProvider{ "terraform": {Source: "terraform.io/builtin/terraform"}, },

That works as expected although it calls terraform init unnecessarily in this very specific case -- I'll look to see if it makes sense to potentially have the init calling logic skip if terraform.io/builtin/terraform is the only external provider and source.

Nice! I've replaced the usage of the time provider with the built-in terraform_data resource in all of the ExpectUnknownOutputValue, and ExpectUnknownOutputValueAtPath plan check tests.

bflad · 2023-11-22T12:58:37Z

website/docs/plugin/testing/acceptance-tests/plan-checks/index.mdx

+- Available plan phases for **Refresh** mode are defined in the [`TestStep.RefreshPlanChecks`](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-testing/helper/resource#TestStep) struct
+- **Import** mode currently does not run any plan operations, and therefore does not support plan checks.
+
+There are built-in [resource plan checks](/terraform/plugin/testing/acceptance-tests/plan-checks/resource), and [output plan checks](/terraform/plugin/testing/acceptance-tests/plan-checks/output).


Resource is definitely the correct word to use for both managed resources and data resources (colloquially called data sources), however I wonder if we should explicitly mention "managed resources and data resources" when we have textual room so folks aren't confused about how to test data sources?

I've updated the Overview and Resource pages to explicitly mention "managed resources, and data sources". I went with "data sources" rather than "data resources" as it seems likely that the former might be more familiar terminology given the documentation on the terraform language for Data Sources page. Does that seem ok?

github-actions · 2023-12-29T01:59:52Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

Implementing expect unknown output value plan check (#219)

46d4550

bendbennett added the enhancement New feature or request label Nov 17, 2023

bendbennett added this to the v1.6.0 milestone Nov 17, 2023

bendbennett added 5 commits November 17, 2023 16:49

Linting (#219)

9c356ef

Merge remote-tracking branch 'origin/main' into bendbennett/issues-219

93fdf68

Implementing expect null output value plan check (#219)

44a3d3e

Updating plan checks docs (#219)

3a3f697

Adding changelog entries (#219)

72fa125

bendbennett marked this pull request as ready for review November 20, 2023 14:40

bendbennett requested a review from a team as a code owner November 20, 2023 14:40

bendbennett changed the title ~~Implementing expect unknown output value plan check~~ Implementing expect unknown and null output value plan checks Nov 20, 2023

SBGoods reviewed Nov 20, 2023

View reviewed changes

bflad reviewed Nov 20, 2023

View reviewed changes

bendbennett and others added 3 commits November 21, 2023 07:34

Apply suggestions from code review

6dc7804

Co-authored-by: Selena Goods <[email protected]>

Updates following code review (#219)

962fd3a

Adding copyright (#219)

f3a183a

bendbennett added 4 commits November 22, 2023 08:41

Splitting unknown and null output value plan checks into Expect<Null|…

8ff2cf6

…Unknown>OutputValue and Expect<Null|Unknown>OutputValueAtPath (#219)

Adding changelog entries (#219)

f147e3b

Updating docs (#219)

6dd8bb2

Adding copyright headers (#219)

011d149

bflad approved these changes Nov 22, 2023

View reviewed changes

bendbennett added 2 commits November 23, 2023 09:30

Updating docs and tests following code review (#219)

155746d

Rewording to explicitly mention data sources (#219)

03de815

bendbennett merged commit 5f5c3b7 into main Nov 28, 2023
6 checks passed

bendbennett deleted the bendbennett/issues-219 branch November 28, 2023 14:30

github-actions bot locked as resolved and limited conversation to collaborators Dec 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing expect unknown and null output value plan checks #220

Implementing expect unknown and null output value plan checks #220

bendbennett commented Nov 17, 2023

SBGoods left a comment

SBGoods Nov 20, 2023

bendbennett Nov 21, 2023 •

edited

Loading

SBGoods Nov 20, 2023

bflad Nov 20, 2023

bendbennett Nov 21, 2023

bflad left a comment

bflad Nov 20, 2023

bflad Nov 20, 2023

bendbennett Nov 21, 2023

bendbennett Nov 22, 2023

bflad Nov 20, 2023

bendbennett Nov 21, 2023

bflad Nov 22, 2023

bflad Nov 22, 2023

bendbennett Nov 23, 2023

bendbennett commented Nov 21, 2023

bflad left a comment

bflad Nov 22, 2023

bflad Nov 22, 2023

bflad Nov 22, 2023

bendbennett Nov 23, 2023

bflad Nov 22, 2023

bendbennett Nov 23, 2023 •

edited

Loading

github-actions bot commented Dec 29, 2023

Implementing expect unknown and null output value plan checks #220

Implementing expect unknown and null output value plan checks #220

Conversation

bendbennett commented Nov 17, 2023

SBGoods left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendbennett Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendbennett commented Nov 21, 2023

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendbennett Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 29, 2023

bendbennett Nov 21, 2023 •

edited

Loading

bendbennett Nov 23, 2023 •

edited

Loading