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.

Sign up for GitHub

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

Jump to bottom

Fix - Add validation for case when isReadOnly and isRequired are true and value empty in config variable #293

Merged
merged 5 commits into from
Mar 28, 2022
Merged

Fix - Add validation for case when isReadOnly and isRequired are true and value empty in config variable #293

merged 5 commits into from
Mar 28, 2022

Conversation

samuel-br
Copy link
Contributor

Issue & Steps to Reproduce / Feature Request

fixes #217

Solution

Add validation func that throw error when is_required and is_read_only are true and value is empty

@samuel-br samuel-br self-assigned this Mar 27, 2022
@samuel-br samuel-br changed the title add validation and UT Add validation for case when isReadOnly and isRequired are true and value empty in config variable Mar 27, 2022
@samuel-br samuel-br changed the title Add validation for case when isReadOnly and isRequired are true and value empty in config variable Fix - Add validation for case when isReadOnly and isRequired are true and value empty in config variable Mar 27, 2022
@eranelbaz eranelbaz requested a review from a team March 27, 2022 13:00
eranelbaz
eranelbaz reviewed Mar 27, 2022
View reviewed changes
Copy link
Member

@eranelbaz eranelbaz left a comment

Choose a reason for hiding this comment

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

What about using ConflictsWith?

@samuel-br
Copy link
Contributor Author

What about using ConflictsWith?

@eranelbaz
ConflictsWith is work as 'or' operator and we need 'and'

@samuel-br
Copy link
Contributor Author

samuel-br commented Mar 27, 2022 •
edited
Loading

also I did try to find a way to check it in the schema itself but it seems to be known issue on TF that there is no validation relating to more then one key

@eranelbaz
Copy link
Member

What about using ConflictsWith?

@eranelbaz ConflictsWith is work as 'or' operator and we need 'and'

Yes, it will say either a or b
and that is what we want, no?

@samuel-br
Copy link
Contributor Author

What about using ConflictsWith?

@eranelbaz ConflictsWith is work as 'or' operator and we need 'and'

Yes, it will say either a or b and that is what we want, no?

no key is conflict with just one of the other keyes

isRequired conflict with isReadOnly and empty value
isReadOnly conflict with isRequired and empty value
empty value conflict with isRequired and isReadOnly

so you cant ensure that with ConflictsWith

@samuel-br
Copy link
Contributor Author

@eranelbaz
hashicorp/terraform-plugin-sdk#233

@samuel-br samuel-br requested a review from eranelbaz March 27, 2022 15:48
eranelbaz
eranelbaz approved these changes Mar 28, 2022
View reviewed changes
Copy link
Member

@eranelbaz eranelbaz left a comment

Choose a reason for hiding this comment

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

LGTM

env0/resource_configuration_variable_test.go Outdated
Comment on lines 696 to 698
//mock.EXPECT().ConfigurationVariablesById(newConfigVar.Id).Return(newConfigVar, nil),
)
// mock.EXPECT().ConfigurationVariableUpdate(client.ConfigurationVariableUpdateParams{CommonParams: updateParams, Id: newConfigVar.Id}).Times(1).Return(configVar, nil)
Copy link
Member

Choose a reason for hiding this comment

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

please remove commented code

@eranelbaz eranelbaz added ready to merge PR approved - can be merged once the PR owner is ready and removed pending final review labels Mar 28, 2022
@samuel-br samuel-br merged commit 1c36827 into main Mar 28, 2022
@samuel-br samuel-br deleted the fix_readonly_and_required_noValue-#217 branch March 28, 2022 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eranelbaz eranelbaz eranelbaz approved these changes

Assignees

@samuel-br samuel-br

Labels
provider ready to merge PR approved - can be merged once the PR owner is ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Should not allow for readonly+required+no value
3 participants
@samuel-br @eranelbaz @yaronya