search

strimzi / kafka-access-operator

Operator for sharing access to Strimzi clusters across namespaces
Apache License 2.0
14 stars 13 forks source link

Add `@Buildable` annotation to all model classes for generating the Builder classes #49

Closed im-konge closed 3 months ago

im-konge commented 3 months ago

This PR adds @Buildable annotation to the model Classes, so the Builder, Fluent, and FluentImpl classes are generated during the project build. The Builder classes are used for testing purposes, but also for creating the KafkaAccess CR using Java code (and not just YAMLs). Finally, it changes the tests to use these new builders.

im-konge commented 3 months ago

I assume the second PR would be the modification of tests?

Yes, I didn't want to include this in #48 and have it separated to other stuff. So I wanted to do the change in all classes in different PR.

im-konge commented 3 months ago

@see-quick good point with the tests, changed them in this PR to use the new builders. Thanks

im-konge commented 3 months ago

Doesn't Fabric8 have its own builders for the CRDs?

The Fabric8 has deps for building the CRDs (from POJOs) and the POJOs (from CRDs), but based on articles I read, these can be extended by Sundrio's extensions and annotations (because the deps are using Sundrio underneath) to generate the Builders.

I will check that more, but from what I saw until now, that is the way how it is used now (but maybe I'm completely wrong of course).

EDIT: the builders are generated during the POJOs generation (based on https://github.com/rohanKanojia/kubernetes-client-demo/blob/master/fabric8-crd-java-generator-demo/README.md).

katheris commented 3 months ago

Other than the discussion about preventing javadoc warnings from failing the build these changes look good to me. I'll hold off approving until that is resolved though

im-konge commented 3 months ago

@scholzj @katheris @mstruk I added two modules - api and operator - and changed everything related to that accordingly. Could you please have another look? Thanks

katheris commented 3 months ago

@im-konge what was the reasoning for putting StatusUtils and StatusUtilsTest inside the api module? I think because the status is only updated by the operator, and the methods there are more around updating the status rather than inspecting it, I would expect those to stay with the other internal classes in the operator module.

I'm also wondering whether the internal package should be renamed to something else, I guess it contains classes for parsing the Kafka CR, but I can't come up with a better name, so happy to leave it as internal for now and we can always rename in future.

im-konge commented 3 months ago

@katheris the reason I moved the StatusUtils there is because those methods are used inside the KafkaAccessStatus, which should be part of the api module. In case that I would leave it in operator module, I would create a cyclic dependency.

Maybe it should not be there and it should be done a bit differently, but I guess that's something that should be changed in some other PR (I'm fine with changing it, but I don't want to make the scope of the PR bigger and bigger).

I'm also wondering whether the internal package should be renamed to something else, I guess it contains classes for parsing the Kafka CR, but I can't come up with a better name, so happy to leave it as internal for now and we can always rename in future.

Sure I can change it as well if there is a better name.

katheris commented 3 months ago

@im-konge ah thanks, that makes sense, yeah lets leave it there for now

katheris commented 3 months ago

Thanks @im-konge for the PR, it looks good to me. I'll leave it a day or so to see if @mstruk has any comments, and if not will go ahead and merge it.