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

Add IsRequired validators to list, set, and object validators for Blocks #107

Merged
merged 6 commits into from
Feb 8, 2023

Conversation

austinvalle
Copy link
Member

@austinvalle austinvalle commented Feb 6, 2023

closes #105

  • This PR introduces 3 new validators that will provide equivalent Required functionality to:
    • ListNestedBlock
    • SetNestedBlock
    • SingleNestedBlock

Example validation error:

Error: Invalid Block

	with data.cloudinit_config.foo,
	on terraform_plugin_test.tf line 1, in data "cloudinit_config" "foo":
	 1: data "cloudinit_config" "foo" {
	
Block part must have a configuration value set as the provider has marked
it as required

https://pkg.go.dev/github.com/hashicorp/terraform-plugin-framework-validators@v0.9.1-0.20230206221608-25bada52026a

Notes

  • All validators referenced Attributes in the error messages, and I updated mine to Block. Not sure if this will cause unnecessary confusion?
  • I originally had a comment noting objectvalidator.IsRequired usage with schema.NestedBlockObject, but looking at it now, it doesn't seem like that would be useful?

@austinvalle austinvalle requested a review from a team as a code owner February 6, 2023 20:52
Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Do we need to give any consideration to the possibility of these validators being used erroneously? For instance:

func (e *exampleResource) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) {
	resp.Schema = schema.Schema{
		Attributes: map[string]schema.Attribute{
			"set_nested_attribute": schema.SetNestedAttribute{
				Optional: true,
				Validators: []validator.Set{
					setvalidator.IsRequired(),
				},
			},
		},
	}
}

@austinvalle
Copy link
Member Author

austinvalle commented Feb 7, 2023

LGTM 👍

Do we need to give any consideration to the possibility of these validators being used erroneously? For instance:

func (e *exampleResource) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) {
	resp.Schema = schema.Schema{
		Attributes: map[string]schema.Attribute{
			"set_nested_attribute": schema.SetNestedAttribute{
				Optional: true,
				Validators: []validator.Set{
					setvalidator.IsRequired(),
				},
			},
		},
	}
}

That's a good question. I think we already have the potential of validators conflicting that we don't prevent:

"example_string": schema.StringAttribute{
	Validators: []validator.String{
		stringvalidator.LengthAtLeast(5),
		stringvalidator.LengthAtMost(2),
	},
},

And as you noted, this PR introduces conflicts between our "built-in" fields Required + Optional. I'd think as long as the behavior is predictable to a provider developer it shouldn't be too much of a concern, but I could be overlooking something.

As well as this validation is a little special, as it's meant only for the Block types, which don't have Required / Optional, but it can still be used anywhere regardless 🤔

@austinvalle
Copy link
Member Author

Bubbled up conversation to our slack, I think this is good to merge for now

@austinvalle austinvalle merged commit f27f636 into main Feb 8, 2023
@austinvalle austinvalle deleted the av/not-null branch February 8, 2023 16:01
@bflad bflad added this to the v0.10.0 milestone Feb 8, 2023
@bflad bflad added the enhancement New feature or request label Feb 8, 2023
@bflad
Copy link
Contributor

bflad commented Feb 8, 2023

Added labels and assigned to upcoming milestone.

@bflad
Copy link
Contributor

bflad commented Feb 8, 2023

austinvalle added a commit that referenced this pull request Feb 8, 2023
* added changelog

* updated changie logs for subsystem
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "not null" validator for listvalidator, setvalidator, and objectvalidator
3 participants