Introducing `schemavalidator` package #32

detro · 2022-05-31T17:38:39Z

Closes #14
Closes #15
Closes #16
Closes #17

NOTE: This PR depends on changes in terraform-plugin-framework that will land in v0.10.0. To allow tests to pass, I have temporarily set the go.mod to pull main branch: to do the v0.4.0 release, we will have to wait on FW to ship, then we will update go.mod, and then cut it.

bendbennett

Nice!

combovalidator/at_least_one_of.go

combovalidator/exactly_one_of_test.go

go.mod

helpers/attributepath/to_string.go

ewbankkit · 2022-06-01T13:04:55Z

Drive-by comment:
Have you considered moving helpers below an internal directory so that the functions aren't available in the public API?

bflad · 2022-06-01T18:14:26Z

I'm not personally sure why they're being moved under helpers/ yet (I haven't asked), but I can say that external validators may wish to implement these as well-defined diagnostics. In the future, we may be able to also leverage them for potential use cases such as a generic Not() validator.

detro · 2022-06-06T10:17:57Z

@ewbankkit @bflad about the move under helpers/ I did touch briefly upon it in the previous (now closed) PR and verbally: the essence being, the role of those functions isn't central to validation (the focus of this package) - they have more of a supporting role in the play. The name helpers/ is just an alternative to utilities/ or other well known terms that should allow the developer to understand what's in the package, without a lot of explanation.

About the new validators, I was reading the doc that @bflad put down for native AttributePath in FW, and I was thinking that the name schemavalidator might be better than combovalidator: what do you think?

bflad · 2022-06-07T20:52:43Z

I may or may not have visceral reactions due to helper/ in terraform-plugin-sdk. 😂 As you mention, it's probably better to put it under there. We're not breaking a cardinal Go convention about avoiding ambiguous package names if its just a prefix.

The schemavalidator package name is a great idea! I'm on board with that naming.

I've mentioned out of band from GitHub that it might be good to wait to merge and release these until hashicorp/terraform-plugin-framework#81 is sorted upstream, just so we don't immediately have to introduce that breaking change here. 👍 I also think you and @bendbennett have been working pretty closely on these, so please reach out if any of my feedback from #29 doesn't make sense.

bflad · 2022-06-22T12:53:32Z

FYI, moving this to v0.4.0 so we can release v0.3.0 with additional validators not dependent on hashicorp/terraform-plugin-framework#81, upgrade the framework dependency when its released, then focus immediately on these.

…ath.Expressions` manipulation

bflad

Overall it's shaping up really good -- I think we should discuss the semantics of unknown values some more though before committing to these as written.

.changelog/42.txt

schemavalidator/doc.go

schemavalidator/at_least_one_of.go

helpers/pathutils/merge.go

schemavalidator/conflicts_with.go

schemavalidator/conflicts_with_test.go

schemavalidator/required_with.go

Co-authored-by: Brian Flad <[email protected]>

Reference: hashicorp/terraform-plugin-framework-validators#32

bflad

Just some additional drive-by notes about the latest code. Happy to have the unknown values discussion anytime.

I also wonder if we should still consider creating DataSourceConfigValidator/ProviderConfigValidator/ResourceConfigValidator implementations of these to suit provider developer styles as noted in the original issues because sometimes basing multiple attribute validation on a particular attribute can be confusing or duplicative (both in code and returned diagnostics), e.g.

"attr1": tfsdk.Attribute{
  Validators: []tfsdk.AttributeValidator{
    schemavalidator.ConflictsWith(path.MatchRoot("attr2")),
  },
}
"attr2": tfsdk.Attribute{
  Validators: []tfsdk.AttributeValidator{
    schemavalidator.ConflictsWith(path.MatchRoot("attr1")),
  },
}

Would return two similar error diagnostics (one for each attribute validator) versus something like:

func (r exampleResource) ConfigValidators(ctx context.Context) []tfsdk.ResourceConfigValidator {
  return []tfsdk.ResourceConfigValidator{
    configvalidator.ConflictingAttributes(
      path.MatchRoot("attr1"),
      path.MatchRoot("attr2"),
    ),
  }
}

Those *ConfigValidator implementations don't need to be part of this PR, but I worry about closing the original issues without at least creating followup ones to support the other declaration style. Refer also to hashicorp/terraform-plugin-sdk#286. I also need to ensure there is an issue in terraform-plugin-framework that allows a single configuration validator to work across all three of DataSource/Provider/Resource; I'm pretty sure the Validate() method of each is conflicting at the moment, which was an oversight in the original implementation.

schemavalidator/at_least_one_of.go

schemavalidator/conflicts_with.go

bflad · 2022-07-07T23:54:33Z

schemavalidator/required_with.go

+	return fmt.Sprintf("Ensure that if an attribute is set, also these are set: %q", av.pathExpressions)
+}
+
+func (av requiredWithAttributeValidator) Validate(ctx context.Context, req tfsdk.ValidateAttributeRequest, res *tfsdk.ValidateAttributeResponse) {


I believe if the current attribute with the validator is null, the rest of the logic can be skipped.

Suggested change

func (av requiredWithAttributeValidator) Validate(ctx context.Context, req tfsdk.ValidateAttributeRequest, res *tfsdk.ValidateAttributeResponse) {

func (av requiredWithAttributeValidator) Validate(ctx context.Context, req tfsdk.ValidateAttributeRequest, res *tfsdk.ValidateAttributeResponse) {

if req.AttributeConfig.IsNull() {

return

}

About this one I'm a bit perplexed.

The issue is, if this is "requiredWith", we need to establish if it's NULL whilst the other it's required to be with are NOT NULL.

It should instead fail when the attribute is NULL, but the one its required to be required with aren't.

bflad · 2022-07-07T23:55:01Z

schemavalidator/required_with.go

+			return
+		}
+
+		if !req.AttributeConfig.IsNull() && mpVal.IsNull() {


Paired with the above check if the current attribute with the validator is null:

Suggested change

if !req.AttributeConfig.IsNull() && mpVal.IsNull() {

if mpVal.IsNull() {

See above, why I'm perplexed with this one.

Reference: hashicorp/terraform-plugin-framework-validators#32

… values are `Unknown`

detro · 2022-07-08T14:32:29Z

In regards to having also the DataSourceConfigValidator / ProviderConfigValidator / ResourceConfigValidator, I think that is a fair and probably necessary thing.

One thing that working on those validators showed me, is that while they fulfil the need and match the SDK featureset, the resource/data-source/provider level ones, for schema validation, are WAY better suited.

So I'm going to do create an additional issue to track the work to add those kinds of validators, so we don't lose track of this need, but we can still close the existing issues I was targeting here.

…ath.Expressions`

bflad

Approving the changes outside my own

github-actions · 2022-08-12T02:35:20Z

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.

detro requested a review from a team as a code owner May 31, 2022 17:38

detro added this to the v0.2.0 milestone May 31, 2022

bendbennett approved these changes Jun 1, 2022

View reviewed changes

combovalidator/at_least_one_of.go Outdated Show resolved Hide resolved

combovalidator/exactly_one_of_test.go Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

helpers/attributepath/to_string.go Outdated Show resolved Hide resolved

detro modified the milestones: v0.2.0, v0.3.0 Jun 7, 2022

detro force-pushed the detro/combovalidator branch from 587307a to 8153099 Compare June 20, 2022 09:30

detro changed the title ~~Introducing combovalidator package~~ Introducing schemavalidator package Jun 20, 2022

detro force-pushed the detro/combovalidator branch from 9c8dd47 to e5e117e Compare June 20, 2022 13:01

detro mentioned this pull request Jun 21, 2022

Introducing OneOf and NoneOf #42

Merged

bflad added the enhancement New feature or request label Jun 22, 2022

bflad modified the milestones: v0.3.0, v0.4.0 Jun 22, 2022

detro force-pushed the detro/combovalidator branch 2 times, most recently from a89d000 to 60886e2 Compare June 27, 2022 17:18

Ivan De Marino added 10 commits July 6, 2022 14:33

Marking unused arguments as such

189f02b

temporary: attributepath helpers

02ff6bf

Introducing schemavalidator package

4a4a7a8

CHANGELOG entry

4a7c9ab

Removing any reference to tftypes.AttributePath in favour of path.*

190e1e7

Bump up to latest version of terraform-plugin-framework

06801b8

Fix changelog entries

398db10

Update helpers with a pathutils sub-package to help with the new `p…

d0f4597

…ath.Expressions` manipulation

Update all schemavalidators to use path.Expressions

a5fdeb7

Updating dependencies

e3b6a99

detro force-pushed the detro/combovalidator branch from 60886e2 to e3b6a99 Compare July 6, 2022 13:36

bflad reviewed Jul 6, 2022

View reviewed changes

Ivan De Marino and others added 4 commits July 7, 2022 11:54

Update .changelog/42.txt

c5d5a41

Co-authored-by: Brian Flad <[email protected]>

Apply suggestions from code review

f19e7d0

Co-authored-by: Brian Flad <[email protected]>

Update schemavalidator/at_least_one_of.go

b6d9209

Co-authored-by: Brian Flad <[email protected]>

PR review feedback

2d96f2a

bflad added a commit to hashicorp/terraform-plugin-framework that referenced this pull request Jul 7, 2022

path: Add Append methods for Paths and Expressions

a725338

Reference: hashicorp/terraform-plugin-framework-validators#32

bflad added a commit to hashicorp/terraform-plugin-framework that referenced this pull request Jul 7, 2022

path: Add Append methods for Paths and Expressions

f1ac931

Reference: hashicorp/terraform-plugin-framework-validators#32

bflad mentioned this pull request Jul 7, 2022

path: Add Append methods for Paths and Expressions hashicorp/terraform-plugin-framework#401

Merged

bflad reviewed Jul 8, 2022

View reviewed changes

bflad mentioned this pull request Jul 8, 2022

Allow Configuration Validators To Satisfy DataSourceConfigValidator, ProviderConfigValidator, and ResourceConfigValidator Interfaces hashicorp/terraform-plugin-framework#404

Closed

bflad added a commit to hashicorp/terraform-plugin-framework that referenced this pull request Jul 8, 2022

path: Add Append methods for Paths and Expressions (#401)

96769e5

Reference: hashicorp/terraform-plugin-framework-validators#32

Ivan De Marino added 2 commits July 8, 2022 14:59

Making the schemavalidator permissive when the involved attributes'…

7e4f004

… values are `Unknown`

Further PR review tweaks

02395ad

bendbennett mentioned this pull request Jul 8, 2022

Adding proxy support hashicorp/terraform-provider-http#154

Open

detro mentioned this pull request Jul 8, 2022

Resource/Data-Source/Provider level Schema Configuration Validators #48

Closed

1 task

Ivan De Marino and others added 4 commits July 8, 2022 16:38

Renamed 'RequiredWith' to 'AlsoRequires', and also updated the godoc

3d5a6f1

Making use of the new .Append() method added to path.Paths and `p…

ee38042

…ath.Expressions`

deps: Update github.com/hashicorp/terraform-plugin-framework@main

259abd4

schemavalidator: Updates to use (Expression).MergeExpressions()

a357afd

bflad approved these changes Jul 12, 2022

View reviewed changes

detro merged commit 81783a7 into main Jul 12, 2022

detro deleted the detro/combovalidator branch July 12, 2022 15:50

github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2022

	if !req.AttributeConfig.IsNull() && mpVal.IsNull() {
	if mpVal.IsNull() {

Introducing schemavalidator package #32

Introducing schemavalidator package #32

Uh oh!

Conversation

detro commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bendbennett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ewbankkit commented Jun 1, 2022

Uh oh!

bflad commented Jun 1, 2022

Uh oh!

detro commented Jun 6, 2022

Uh oh!

bflad commented Jun 7, 2022

Uh oh!

bflad commented Jun 22, 2022

Uh oh!

bflad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bflad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bflad Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

detro Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

bflad Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

detro Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

detro commented Jul 8, 2022

Uh oh!

bflad left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Introducing `schemavalidator` package #32

Introducing `schemavalidator` package #32

detro commented May 31, 2022 •

edited

Loading