Closed aygold92 closed 4 years ago
even if the default implementation honours order, it doesn't necessarily mean we should change this flag, although i can get behind this because it's less confusing. does this have any impact on code generation?
at the moment it does not affect code generation because we don't use it anywhere, but as we discussed offline I think we should be adding ordered set and multiset to the codegen options. Why should we not change the flag to true if the actual default interpretation by CFN is true?
I don't think it's true it isn't used: https://github.com/aws-cloudformation/aws-cloudformation-rpdk/blob/185bff46ee11dcde384541f1dc769612c02df991/src/rpdk/core/jsonutils/resolver.py#L112-L117
Oh interesting, I guess it defaults to False there? It should default to true though
I don't think it's true it isn't used: https://github.com/aws-cloudformation/aws-cloudformation-rpdk/blob/185bff46ee11dcde384541f1dc769612c02df991/src/rpdk/core/jsonutils/resolver.py#L112-L117
Oh interesting, I guess it defaults to False there? It should default to true though
I am fixing that
cheers. that logic is gnarly, i think going from the unit tests what the result is is the right move, just pointing out it is being used, not necessarily the logic is wrong. it may have been done that way so that uniqueItems: true
and insertionOrder
not specified works.
cheers. that logic is gnarly, i think going from the unit tests what the result is is the right move, just pointing out it is being used, not necessarily the logic is wrong. it may have been done that way so that
uniqueItems: true
andinsertionOrder
not specified works.
yeah, that's why in the description for this PR, I mentioned we should consider changing the property name so that it can default to false, which would fix that
Description reasoning feels somewhat circular to me, don't completely follow why the default
definition switched to true
rather than the implementation's default
switching to false
to match the previous definition
Less than 200 of the 1000+ public resource schema arrays
specified insertionOrder
, and insertionOrder
defaulting to true
is one of the main issues with drift detection for Registry types causing false positives
Seems easier to default
to false
than modifying most resource schemas to specify insertionOrder
for drift detection to work properly
Description reasoning feels somewhat circular to me, don't completely follow why the
default
definition switched totrue
rather than the implementation'sdefault
switching tofalse
to match the previous definitionSeems easier to
default
tofalse
than modifying most resource schemas to specifyinsertionOrder
for drift detection to work properly
Not sure what you mean by this. By default, order is honored, and if they have a Set then they need to say insertionOrder: false
. It would have been better to call this something where the default could be false, but in terms of updating schemas that have not included it, it doesn't really make a difference. In most cases, teams don't need to include it because by default we honor the order
Issue #, if available:
Description of changes: The default value for insertionOrder should be true, because by default we will honor order (i..e, we assume list by default, rather than set). This kind of makes me think the property should actually be called something else, so that the default value can be false, but I'm not sure we can make that change at this point. If so, would like to have suggestions (my original idea was "orderMatters", which allows us to default to false, but that sounds bad)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.