Fix #5388: Generate deterministic CRDs

[Donnerbart]

Description

Fixes #5388

I'm not sure if you want the provided Complex custom resource example as is in your code base, since it has nested ObjectMetaData, StatefulSetSpec and ServiceSpec and is tested against a checked in YAML file. So on every change on those K8s resources, the checked in file needs to be adapted (for example, non-breaking changes like adding new fields).

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change
• Chore (non-breaking change which doesn't affect codebase;
  test, version modification, documentation, etc.)

Checklist

• Code contributed by me aligns with current project license: Apache 2.0
• I Added CHANGELOG entry regarding this change
• I have implemented unit tests to cover my changes
• I have added/updated the javadocs and other documentation accordingly
• No new bugs, code smells, etc. in SonarCloud report
• I tested my code in Kubernetes
• I tested my code in OpenShift

[andreaTP]

@Donnerbart thanks a lot for taking the time to go through this!

I have a couple of questions but let me know your thoughts!

[andreaTP]

LGTM but I wish @metacosm can do a final review here.

[andreaTP]

@shawkins input would be great too, at least to verify that those changes are not affecting somehow the KC operator.

[Donnerbart]

I've changed the comparison for JSONSchemaProps to use type first, since id doesn't seem to be populated (but still fallback to the id if type is the same value).

I also added more JavaDoc to the DeterministicObjectMapper and its builder class.

[Donnerbart]

The license check is failing for the checked in YAML file in the test resources.
Since the file is used for a byte-wise comparison, it cannot be changed.

I see that there are already some exclusions for test resources:

            <exclude>kubernetes-client/src/test/resources/**/*</exclude>
            <exclude>kubernetes-client/src/test/resources/mockito-extensions/*</exclude>
            <exclude>**/src/test/resources/ssl/*</exclude>
            <exclude>**/src/test/resources/io/fabric8/java/generator/approvals/*.*.approved.txt</exclude>
            <exclude>**/it/**/expected/**/*.expected</exclude>

I don't want to change <exclude>kubernetes-client/src/test/resources/**/*</exclude> to match all modules, so I've added <exclude>**/src/test/resources/*.yml</exclude>, which is hopefully narrow enough to not violate the license requirements.

[shawkins]

@andreaTP @Donnerbart I'll double check specifically with keycloak generation, but glancing at the changes I don't see anything that is too concerning. However there is a lot of complexity that doesn't seem to be needed - with all of the updates to the mapper the only unsorted array in the complex json schema that I see are enum properties. Also printer columns are already sorted by path. Could we consider reducing the changes to 3ea9b08

Or do you have more examples of where arrays / collections end up unsorted in the jsonschema?

[Donnerbart]

@shawkins That's a great finding! I've double checked our complex example, since there were for sure more nondeterministic fields. Looks like these were all properties, that are now sorted via .configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true).

So indeed, with the correctly sorted enums that quite hacky way of adding the collection serializer is not needed anymore.

I also cleaned up the complex example, by adding simplified, but stable versions of the K8s models being used. This still provides enough properties, that were unsorted before, but won't break on future changes on the K8s model classes.

[andreaTP]

@Donnerbart the CI failure on Win seems to be relevant, probably the encoding of the file is causing a difference.
I would be completely ok if, instead of comparing byte-by-byte we compare with String::equal the full lines.

This approach might also play nicer with the License check, e.g. you can include the License header in the committed file and skip it during the comparison.

[Donnerbart]

@andreaTP Yeah, when I saw Windows I had the same thought. Will look into this first thing tomorrow (it's already end of workday for me).

I guess you don't want any helper dependency like Apache Commons or so in the test scope, so I'll write that comparison by hand. Will also look into adding the license header then.

[andreaTP]

Thanks for getting back, no rush!

I guess you don't want any helper dependency like Apache Commons or so in the test scope, so I'll write that comparison by hand.

I (personally) have no issues with adding helper dependencies to the test scope ...

[Donnerbart]

• Removed the license check exclusion for YAML files in test resources.
• Added the license header to the checked in YAML file.
• Changed the file comparison to BufferedReader::readLine (that should handle all types of newlines).
• Skipped leading comments and newlines for file comparison.
• Improved the JavaDoc on the Complex example classes.
• Fixed the description in CHANGELOG.md.

[shawkins]

LGTM, just one minor comment.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

100.0% Coverage
0.2% Duplication

The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

[manusa]

LGTM, thx!