fix(crd-generator): default values for CRD fields can be numeric or boolean #6666
Conversation
…oolean Signed-off-by: Marc Nuri <[email protected]>
manusa
requested review from
andreaTP,
metacosm,
oscerd,
rohanKanojia and
shawkins
as code owners
|
| @@ -42,6 +42,15 @@ public class AnnotatedSpec { | |||
| private String defaultValue; | |||
| @Default("my-value2") | |||
| private String defaultValue2; | |||
| @JsonProperty(defaultValue = "true") | |||
There was a problem hiding this comment.
What happens when the two annotations contain different values?
There was a problem hiding this comment.
There's a precedence test for this, you can check it out ;)
JsonProperty takes precedence since AFAIR the other annotation will eventually be removed or deprecated.
There was a problem hiding this comment.
AFAIR the other annotation will eventually be removed or deprecated
oh, really? I'm surprised!
Should this behavior be documented? somewhere
There was a problem hiding this comment.
I have really no idea, I deduced this from this TODO comment:
I thought this had been discussed, but maybe not.
TBH this is the first time I'm actually looking into the CRD generator, up until now I had been delegating any task related to the CRD generator to you folks. So I might be disconnected.
In any case, a deprecation would happen in 7.x and not in 7.0.0
| assertThat(JsonSchema.from(ClassInTest.class).getProperties()) | ||
| .extracting("defaultValueForInt._default") | ||
| .asInstanceOf(InstanceOfAssertFactories.type(JsonNode.class)) | ||
| .isInstanceOf(LongNode.class) |
There was a problem hiding this comment.
A little surprised this is a LongNode
There was a problem hiding this comment.
This has to do with the Number parser which considers this to be of long precision.
It really doesn't matter, since what we want is the number to be considered a number and not a String.
Of course, there must be corner cases such as users declaring a field of type in and setting a default value of 999999999999999999999. My expectation is that the cluster won't accept that CRD.
We could try and consider those corner cases, but if we introduce validations for the default values too, the complexity will be kind of crazy for such a simple feature.
Fixing this is just a matter of using the specific parser for the target type. i.e. adding more if-else clauses.
There was a problem hiding this comment.
Good enough for me, if we get any complaints over this we should remember to add checks for the boundaries (i.e. Integer.MAX_VALUE) etc.
| .asInstanceOf(InstanceOfAssertFactories.type(JsonNode.class)) | ||
| .isInstanceOf(LongNode.class) | ||
| .extracting(JsonNode::asLong) | ||
| .isEqualTo(1337L); |
There was a problem hiding this comment.
Would it be possible to also generate the resulting YAML file?
I'm interested in checking the L and d suffixes
There was a problem hiding this comment.
The suffixes here I added them to highlight that these values are equal to the expected type, to make it clear that a long is a long and a double is not a double.
How it renders in the CRD is not about how the value is stored in a JsonNode, but how the serializer deals with those. Basically they'll be rendered as any other number in a JSON->YAML document.
There's also an approval test modified that you can check:
I noticed that I didn't add a specific test for doubles, but the expected value should be an unquoted number with a . in case it has decimals.
Description
Fixes #6654
Type of change
test, version modification, documentation, etc.)
Checklist