-
Notifications
You must be signed in to change notification settings - Fork 19
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 plugin framework conflics_with and at_least_one_of to instance #181
Conversation
Closes #180 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we would conclude to move ahead with this PR, please also remove the function atMostOneOf
, since it is no longer used anywhere.
@@ -116,6 +117,21 @@ func (r InstanceResource) Schema(_ context.Context, _ resource.SchemaRequest, re | |||
PlanModifiers: []planmodifier.String{ | |||
stringplanmodifier.RequiresReplace(), | |||
}, | |||
Validators: []validator.String{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the formatting with gofmt
. In Go indentation is done with tab
(and not spaces).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
path.MatchRoot("source_file"), | ||
}... | ||
), | ||
stringvalidator.AtLeastOneOf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation enforces atMostOneOf
, which is not the same as AtLeastOneOf
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the AtLeastOneOf
validator. I think ConflictsWith
is enough to replicate atMostOneOf
.
@Iduoad do you would like to have some help to finish this pull request? |
Validators: []validator.Object{ | ||
objectvalidator.ConflictsWith( | ||
path.Expressions{ | ||
path.MatchRoot("image"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be omitted since the ConflictWith validation is already defined in the image. Kept it for readability.
Not sure if is this the correct way to do it.
@maveonair I rebased on the latest commit on main, run gofmt and went through the changes again. PR is ready for review. |
ec8dec3
to
3fca061
Compare
* Replaces manual mutual exclusiveness validation with ConflictsWith Validator * Upgrade terraform-plugin-framework-validators module to 0.17.0 to fix validation bug. hashicorp/terraform-plugin-framework-validators#251 Signed-off-by: Mohammed Daoudi <[email protected]>
Fixed the failing test and added the signoff in the commit message. |
@maveonair I'll leave it to you to review/merge, just nudged the tests into all going green :) |
I added declarative validator built-in the provider framework instead of imperatively checking in Validate function.