Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Jun 3, 2024

Conversation

im-konge
Copy link
Member

@im-konge im-konge commented May 22, 2024

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 im-konge force-pushed the kafkaaccess-builder branch from 906594b to cd0ccb7 Compare May 22, 2024 15:57
@im-konge im-konge requested review from katheris, mstruk and a team May 22, 2024 15:58
@im-konge im-konge self-assigned this May 22, 2024
@im-konge im-konge added this to the 0.0.1 milestone May 22, 2024
@im-konge im-konge changed the title Add @Buildable annotation to all model classes for generating the Builder classes Add @Buildable annotation to all model classes for generating the Builder classes May 22, 2024
Copy link
Member

@see-quick see-quick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Signed-off-by: Lukas Kral <[email protected]>
@im-konge
Copy link
Member Author

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
Copy link
Member Author

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

@im-konge im-konge requested a review from see-quick May 22, 2024 17:59
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pom.xml Outdated Show resolved Hide resolved
@im-konge
Copy link
Member Author

im-konge commented May 22, 2024

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
Copy link
Contributor

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
Copy link
Member Author

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

im-konge added 2 commits May 28, 2024 14:41
@im-konge im-konge requested a review from scholzj May 28, 2024 14:37
.checkstyle/suppressions.xml Outdated Show resolved Hide resolved
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming it wortks.

@katheris
Copy link
Contributor

@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
Copy link
Member Author

im-konge commented May 29, 2024

@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.

Co-authored-by: Maros Orsak <[email protected]>
Signed-off-by: Lukáš Král <[email protected]>
@katheris
Copy link
Contributor

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

@katheris
Copy link
Contributor

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.

@katheris katheris merged commit 587a5db into strimzi:main Jun 3, 2024
8 checks passed
@im-konge im-konge deleted the kafkaaccess-builder branch June 3, 2024 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants