goldmansachs / crdtools Goto Github PK

ToYaml has the ability to generate yaml from instances of the generated java classes. This piece of functionality is general purpose and could migrate into this repository

SourceGenFromSpec.extractSpecs() implements a transform of a general purpose data structure (CRD) into another data structure (a valid openapi 3.0 specification). This is functionality that lends itself well to unit testing and can be used for all kinds of input. However, the method hard codes its input as the thing returned from getCrdsYaml().

Modify the method such that it specifies input and output as parameters, instead of having the input be hard coded.

Similarly to the :spec target, this target (and it's dependencies should be moved to this repository) should be moved to this repository

The //kccapi:spec target can be built in the monorepo. Migrate the code needed to be able to run this target in this project instead. When this is done, it should be possible to run bazel build //:spec in this repository, generating an output file that is equal to the file available as bazel-bin/kccapi/all-specs.yaml after having run bazel build //kccapi:spec

The signature and semantics of SourceGenFromSpec.generateSourceInDir() are a bit strange, I think it could be improved.

Some things I find strange:

• The name of the method. I read generateSourceInDir() as "generate source, but do it in a specific dir". To do things in a directory, I would assume, means do it with a process that has it's current working directory (CWD) set to some specific directory. This does not make sense in this context.
• The temporary directory to use for output seems to me like an implementation detail, and could be hidden away from the user.
• RuntimeExceptions are not checked and don't need to be declared in the throws statement
• The docs doesn't specify what the format of the output is, only that it will be written to a path

Cloning a fresh copy of the repository and running all test tests fails:

git clone [email protected]:goldmansachs/crdtools.git
cd crdtools
bazel test //...

Test failures:

  JUnit Jupiter:SourceGeneratorTest:Nested class for testing files generation:Test that the all-specs.yaml file has been generated successfully.
    MethodSource [className = 'com.gs.crdtools.SourceGeneratorTest$FileGenerationTest', methodName = 'testSpecFileGeneration', methodParameterTypes = '']
    => org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
       org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
       org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40)
       org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:35)
       org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:179)
       com.gs.crdtools.SourceGeneratorTest$FileGenerationTest.testSpecFileGeneration(SourceGeneratorTest.java:69)
       java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
       java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
       java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
       java.base/java.lang.reflect.Method.invoke(Method.java:568)
       org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:725)
       [...]
  JUnit Jupiter:SourceGeneratorTest:Nested class for testing files generation:Test that the all-specs-only.yaml file has been generated successfully.
    MethodSource [className = 'com.gs.crdtools.SourceGeneratorTest$FileGenerationTest', methodName = 'testAllSpecsOnlyFileGeneration', methodParameterTypes = '']
    => org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
       org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
       org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40)
       org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:35)
       org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:179)
       com.gs.crdtools.SourceGeneratorTest$FileGenerationTest.testAllSpecsOnlyFileGeneration(SourceGeneratorTest.java:91)
       java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
       java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
       java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
       java.base/java.lang.reflect.Method.invoke(Method.java:568)
       org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:725)
       [...]

Besides the openapi spec we need the group and version from the crd, to be able to correctly generate the annotation in the generated models. Let's return a record instead of a String from pullOpenapiSpecs, containing all 3 of the pieces of information needed.

The code currently generates an openapi spec yaml as a temporary file on disk from the input crd, which is then passed to the swagger codegen configurator. I think it would be neater if we could avoid the temporary file (and thus avoid the need to figure out where it should be put, making sure that it is cleaned up and so on) and instead generate the yaml into a string and pass it to swagger in this form.

It would make sense to have a single main method that can be used to connect these two pieces of functionality (generate openapi-v3 spec from crd, and generate java from openapiv3 spec) into a single operation

A crd can (and often does) contain different versions of the models. Right now we just pick the first one, regardless of which one is the latest one. It would be better if we picked the one that has storage: true as a property when generating the openapi spec.

When generating classes from the kccapi upstream crds.yaml the generated class StorgeBucketSpecCondition used to have a field age with the type Long. Now the same field has the type Integer.

Figure out why this is, and come up with a plan to reconcile this (either have crdtools behave as the old implementation, or update the call sites to use the new implementation)

The io.swagger.codegen.v3:swagger-codegen-cli library is built to be able to be used as as single dependency, where everything is baked in such that it can be used as a single jar. This is a bit inconvenient for us, it would be neater to figure out and possibly replicate what swagger-codegen-cli does using swagger-codegen instead.

I don't know whether it is a feature of swagger codegen to add stuff to the generated java code, but it would be very valuable to add information in some way to the generated classes that is needed to be able to generate yaml from instances. Right now, there are some pieces of information in ToYaml.java (The Kind enum) that needs to be updated as soon as one adds a new piece of functionality.