Feature: AtLeastOneOf and ExactlyOneOf validation flags added to schema
#225
Conversation
|
|
| return nil | ||
| } | ||
|
|
||
| allKeys := removeDuplicates(append(schema.AtLeastOneOf, k)) |
There was a problem hiding this comment.
my brain is tingling about this LOC in both functions, but if the tests pass I'm sure it all makes sense.
There was a problem hiding this comment.
This was added to ensure that users coming from the ConflictsWith pattern where you specify every key but the attribute you're in are covered. For example:
"foo": {
ConflictsWith: ["bar"]
}
"bar": {
ConflictsWith:["foo"]
}
We wanted users to go a different route where you specify the whole list:
"foo": {
AtLeastOneOf: ["foo", "bar"]
}
"bar": {
AtLeastOneOf:["foo", "bar"]
}
but we don't have anyway to enforce that they use this pattern so we add the current attribute key and then remove duplicates to ensure our validation doesn't go sideways
| } | ||
|
|
||
| allKeys := removeDuplicates(append(schema.ExactlyOneOf, k)) | ||
| sort.Strings(allKeys) |
There was a problem hiding this comment.
This is for nicer error messages. These have the potential to stack up on one another so if we sort them, they will line up nicely with one another
helper/schema/schema.go
Outdated
| for _, exactlyOneOfKey := range allKeys { | ||
| if raw, ok := c.Get(exactlyOneOfKey); ok { | ||
| if raw == hcl2shim.UnknownVariableValue { | ||
| // An unknown value might become unset (null) once known, so |
There was a problem hiding this comment.
This sorta makes me concerned for some weird edgecase bugs. These are larger Terraform questions like when does validate get called and how often?
There was a problem hiding this comment.
Agreed, however this code has been around for awhile and was copy pasted to be in line with ConflictsWith
There was a problem hiding this comment.
Terraform Core calls ValidateResourceChange again during apply so we can catch things like this. It does mean that there is the potential for an error not to show up until apply time, but that is generally true for all validation.
| } | ||
|
|
||
| allKeys := removeDuplicates(append(schema.AtLeastOneOf, k)) | ||
| sort.Strings(allKeys) |
There was a problem hiding this comment.
same as above do we need to sort?
There was a problem hiding this comment.
I'm guessing it makes for nicer printout in the error message having the keys being sorted?
There was a problem hiding this comment.
Overall approve!! I had some general comments as I load the context of all this into my brain. Seems like a very useful feature! Would be good to get another set of 👀 from @kmoe or @radeksimko
helper/schema/schema.go
Outdated
| return result | ||
| } | ||
|
|
||
| func validateExactlyOneAttributes( |
There was a problem hiding this comment.
Nitpick: Shouldn't this be called validateExactlyOneAttribute?
helper/schema/schema.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func validateAtLeastOneAttributes( |
There was a problem hiding this comment.
Nitpick: Shouldn't this be called validateAtLeastOneAttribute?
|
|
|
@mbfrahry Can you fixup/squash the commits a bit? to a few meaningful commits or just one? Once this is merged We need a changelog entry describing this new feature |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
This PR is looking to add additional validation checks that occur at apply time versus the current method which is to write these checks manually and occur during an apply which has the potential to cause terraform to error out mid run.
ExactlyOneOf is a set of schema keys that, when set, only one of the keys in that list can be specified. It will error if none are specified as well.
AtLeastOneOf is a set of schema keys that, when set, at least one of the keys in that list must be specified.