Split the classes from the io.strimzi.api.kafka.model package to multiple sub-packages #9207

ShubhamRwt · 2023-10-04T19:53:22Z

Type of change

Select the type of your PR

Refactoring

Description

This PR fixes #8628. This PR moves moves classes to their corresponding packages. This PR adds import order checkstyle rule which makes sure that the imports are ordered properly

Checklist

Please go through this checklist and make sure all applicable tasks have been done

Write tests
Make sure all tests pass
Update documentation
Check RBAC rights for Kubernetes / OpenShift roles
Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
Reference relevant issue(s) and close them after merging
Update CHANGELOG.md
Supply screenshots for visual changes, such as Grafana dashboards

scholzj

For lot of the files, the ordering of the imports seems to be complete mess. I wonder if we should use this as an opportunity where we already touch so many files to enforce some import ordering by checkstyle (if that is possible)

api/src/main/java/io/strimzi/api/kafka/model/common/CustomResourceConditions.java

scholzj · 2023-10-05T14:40:28Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaBridgeCluster.java

+import io.strimzi.api.kafka.model.common.CertSecretSource;
+import io.strimzi.api.kafka.model.common.ClientTls;
+import io.strimzi.api.kafka.model.common.JvmOptions;
+import io.strimzi.api.kafka.model.bridge.KafkaBridge;
+import io.strimzi.api.kafka.model.bridge.KafkaBridgeAdminClientSpec;
+import io.strimzi.api.kafka.model.bridge.KafkaBridgeConsumerSpec;
+import io.strimzi.api.kafka.model.bridge.KafkaBridgeHttpConfig;
+import io.strimzi.api.kafka.model.bridge.KafkaBridgeProducerSpec;
+import io.strimzi.api.kafka.model.bridge.KafkaBridgeResources;
+import io.strimzi.api.kafka.model.bridge.KafkaBridgeSpec;
+import io.strimzi.api.kafka.model.common.Rack;
+import io.strimzi.api.kafka.model.common.authentication.KafkaClientAuthentication;
+import io.strimzi.api.kafka.model.common.template.ContainerTemplate;
+import io.strimzi.api.kafka.model.common.template.DeploymentStrategy;
+import io.strimzi.api.kafka.model.common.template.DeploymentTemplate;
+import io.strimzi.api.kafka.model.common.template.InternalServiceTemplate;
+import io.strimzi.api.kafka.model.bridge.KafkaBridgeTemplate;
+import io.strimzi.api.kafka.model.common.template.PodDisruptionBudgetTemplate;
+import io.strimzi.api.kafka.model.common.template.PodTemplate;
+import io.strimzi.api.kafka.model.common.template.ResourceTemplate;
+import io.strimzi.api.kafka.model.common.tracing.Tracing;


Should we have some ordering of the imports? They seem to be mixing up pretty badly.

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

scholzj

LGTM, Thanks.

scholzj

Actually, does not LGTM -> you have to fix the tests. At least some of them are IMHO failing because you have to rename some resources etc.

scholzj · 2023-10-11T20:15:30Z

But the imports and core changes itself look good to me.

ShubhamRwt · 2023-10-19T12:42:28Z

@scholzj Is there a way to restart these tests?

scholzj · 2023-10-19T16:30:22Z

@ShubhamRwt I restarted them.

scholzj · 2023-10-21T14:37:50Z

CHANGELOG.md

@@ -43,6 +43,7 @@
    config.providers.env.class: org.apache.kafka.common.config.provider.EnvVarConfigProvider
    # ...
  ```
+* The `api` module was refactored and classes were moved to new packages


You have to move this to 0.39.0.

scholzj · 2023-10-21T17:26:57Z

/azp run regression

azure-pipelines · 2023-10-21T17:27:08Z

Azure Pipelines successfully started running 1 pipeline(s).

ppatierno

I left one question but no need to change ... but my biggest doubt is ...
should we really have io.strimzi.api.kafka.model and not just io.strimzi.api.model?
I mean, we have a package like io.strimzi.api.kafka.model.kafka.* so kafka is twice in the hierarchy.

ppatierno · 2023-10-23T18:49:25Z

api/src/main/java/io/strimzi/api/kafka/model/acl/AclOperation.java

@@ -2,7 +2,7 @@
 * Copyright Strimzi authors.
 * License: Apache License 2.0 (see the file LICENSE or http://apache.org/licenses/LICENSE-2.0.html).
 */
-package io.strimzi.api.kafka.model;
+package io.strimzi.api.kafka.model.acl;


just an idea, doesn't it make more sense having under user package so ... io.strimzi.api.kafka.model.user.acl?

Yeah that was what we thought at start. But if you go through this sheet -> https://docs.google.com/document/d/1uZwYGZFo9FIR-95q2PV5Npl6IN1KfWSr12cRp21VMpM/edit, I made this change based on Tom's comment

@ppatierno WDYT about this? Do you want @ShubhamRwt to change or or are you fine with it?

Tbh I didn't get what @tombentley meant with that comment :-)

@tombentley Hi, should we have the acl's under user package?

ShubhamRwt · 2023-11-02T15:58:30Z

Update for community call: I was waiting for reviews regarding the new structure so that I can push those changes and rebase this PR together since the merge conflicts keeps happening when other PRs are merged due to dependency import changes

tombentley

@ShubhamRwt thanks for this! Sorry for not having taken a look sooner. There are some build conflicts (which are probably my fault for not having reviewed sooner) which will need to be fixed.

We should consider adding to the rules in .checkstyle/import-control.xml, which currently only really enforces a few things in the operator, not anything in the api module. For example (these are off the top of my head, so might not actually be correct; treat them as illustrative):
* each of the CRD-specific packages in the model module is allowed import the common one, but not vice versa.
* the certificate-manager shouldn't be allowed to import any other Strimzi code

tombentley · 2023-11-20T03:53:10Z

api/pom.xml

+                                <argument>io.strimzi.api.kafka.model.bridge.KafkaBridge=${project.basedir}${file.separator}..${file.separator}packaging${file.separator}install${file.separator}cluster-operator${file.separator}046-Crd-kafkabridge.yaml</argument>
+                                <argument>io.strimzi.api.kafka.model.connector.KafkaConnector=${project.basedir}${file.separator}..${file.separator}packaging${file.separator}install${file.separator}cluster-operator${file.separator}047-Crd-kafkaconnector.yaml</argument>
+                                <argument>io.strimzi.api.kafka.model.mirrormaker2.KafkaMirrorMaker2=${project.basedir}${file.separator}..${file.separator}packaging${file.separator}install${file.separator}cluster-operator${file.separator}048-Crd-kafkamirrormaker2.yaml</argument>
+                                <argument>io.strimzi.api.kafka.model.balancing.KafkaRebalance=${project.basedir}${file.separator}..${file.separator}packaging${file.separator}install${file.separator}cluster-operator${file.separator}049-Crd-kafkarebalance.yaml</argument>


This stands out as the only package name that's a gerund. We don't have connecting, bridging or mirrormaking. Perhaps this should be rebalance instead?

scholzj · 2023-11-30T16:04:14Z

@ShubhamRwt @tombentley Where do we stand on with this PR?

ShubhamRwt · 2023-12-11T06:59:39Z

@scholzj @tombentley What do you suggest, should I make io.strimzi.api.kafka.model to just io.strimzi.api.model as suggested by Paolo?

scholzj · 2023-12-11T09:01:07Z

I would not go for such a major change given this is essentially a part of an public API.

tombentley

LGTM, thanks @ShubhamRwt

ppatierno · 2024-01-08T07:36:43Z

@ShubhamRwt I could have another pass on this but I think you should first resolve all the conflicts.

Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>

ShubhamRwt · 2024-01-08T09:14:19Z

@ppatierno Hi, I have rebased it now

ShubhamRwt force-pushed the apiRefactor branch 2 times, most recently from 2a91372 to b8f85ef Compare October 4, 2023 19:57

ShubhamRwt changed the title ~~Split the classes from the io.strimzi.api.kafka.model package to multiple sub-packages~~ [WIP]Split the classes from the io.strimzi.api.kafka.model package to multiple sub-packages Oct 4, 2023

ShubhamRwt force-pushed the apiRefactor branch from b8f85ef to fbb61ce Compare October 5, 2023 12:24

ShubhamRwt requested review from scholzj, ppatierno and tombentley October 5, 2023 12:25

ShubhamRwt marked this pull request as ready for review October 5, 2023 12:25

scholzj changed the title ~~[WIP]Split the classes from the io.strimzi.api.kafka.model package to multiple sub-packages~~ Split the classes from the io.strimzi.api.kafka.model package to multiple sub-packages Oct 5, 2023

scholzj reviewed Oct 5, 2023

View reviewed changes

ShubhamRwt force-pushed the apiRefactor branch from c7ee75e to 840d188 Compare October 11, 2023 12:39

ShubhamRwt requested a review from scholzj October 11, 2023 12:40

scholzj added this to the 0.39.0 milestone Oct 11, 2023

scholzj approved these changes Oct 11, 2023

View reviewed changes

scholzj reviewed Oct 11, 2023

View reviewed changes

scholzj self-requested a review October 11, 2023 20:15

ShubhamRwt force-pushed the apiRefactor branch from 14c0e31 to 7af4f9f Compare October 19, 2023 10:20

scholzj reviewed Oct 21, 2023

View reviewed changes

ShubhamRwt force-pushed the apiRefactor branch from f833fb9 to c719477 Compare October 23, 2023 11:28

ppatierno reviewed Oct 23, 2023

View reviewed changes

tombentley reviewed Nov 20, 2023

View reviewed changes

ShubhamRwt force-pushed the apiRefactor branch from c719477 to afd7bdd Compare December 6, 2023 07:24

ShubhamRwt force-pushed the apiRefactor branch from 3b4a460 to c7667bb Compare December 11, 2023 13:17

ShubhamRwt requested review from ppatierno and tombentley December 12, 2023 06:47

scholzj modified the milestones: 0.39.0, 0.40.0 Dec 13, 2023

tombentley approved these changes Jan 3, 2024

View reviewed changes

ShubhamRwt added 4 commits January 8, 2024 14:22

Rebase

4c400c8

Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>

Rebased

0553767

Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>

Added changes

cbb2f6b

Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>

Rebased

4a33a94

Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>

ShubhamRwt force-pushed the apiRefactor branch from c7667bb to 4a33a94 Compare January 8, 2024 09:12

ppatierno approved these changes Jan 8, 2024

View reviewed changes

scholzj merged commit 5b7a7d7 into strimzi:main Jan 8, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split the classes from the io.strimzi.api.kafka.model package to multiple sub-packages #9207

Split the classes from the io.strimzi.api.kafka.model package to multiple sub-packages #9207

ShubhamRwt commented Oct 4, 2023 •

edited

scholzj left a comment

scholzj Oct 5, 2023

scholzj left a comment

scholzj left a comment

scholzj commented Oct 11, 2023

ShubhamRwt commented Oct 19, 2023

scholzj commented Oct 19, 2023

scholzj Oct 21, 2023

scholzj commented Oct 21, 2023

azure-pipelines bot commented Oct 21, 2023

ppatierno left a comment

ppatierno Oct 23, 2023

ShubhamRwt Oct 25, 2023 •

edited

scholzj Nov 16, 2023

ppatierno Nov 22, 2023

ShubhamRwt Nov 27, 2023

ShubhamRwt commented Nov 2, 2023 •

edited

tombentley left a comment

tombentley Nov 20, 2023

scholzj commented Nov 30, 2023

ShubhamRwt commented Dec 11, 2023

scholzj commented Dec 11, 2023

tombentley left a comment

ppatierno commented Jan 8, 2024

ShubhamRwt commented Jan 8, 2024 •

edited

Split the classes from the io.strimzi.api.kafka.model package to multiple sub-packages #9207

Split the classes from the io.strimzi.api.kafka.model package to multiple sub-packages #9207

Conversation

ShubhamRwt commented Oct 4, 2023 • edited

Type of change

Description

Checklist

scholzj left a comment

Choose a reason for hiding this comment

scholzj Oct 5, 2023

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

scholzj commented Oct 11, 2023

ShubhamRwt commented Oct 19, 2023

scholzj commented Oct 19, 2023

scholzj Oct 21, 2023

Choose a reason for hiding this comment

scholzj commented Oct 21, 2023

azure-pipelines bot commented Oct 21, 2023

ppatierno left a comment

Choose a reason for hiding this comment

ppatierno Oct 23, 2023

Choose a reason for hiding this comment

ShubhamRwt Oct 25, 2023 • edited

Choose a reason for hiding this comment

scholzj Nov 16, 2023

Choose a reason for hiding this comment

ppatierno Nov 22, 2023

Choose a reason for hiding this comment

ShubhamRwt Nov 27, 2023

Choose a reason for hiding this comment

ShubhamRwt commented Nov 2, 2023 • edited

tombentley left a comment

Choose a reason for hiding this comment

tombentley Nov 20, 2023

Choose a reason for hiding this comment

scholzj commented Nov 30, 2023

ShubhamRwt commented Dec 11, 2023

scholzj commented Dec 11, 2023

tombentley left a comment

Choose a reason for hiding this comment

ppatierno commented Jan 8, 2024

ShubhamRwt commented Jan 8, 2024 • edited

ShubhamRwt commented Oct 4, 2023 •

edited

ShubhamRwt Oct 25, 2023 •

edited

ShubhamRwt commented Nov 2, 2023 •

edited

ShubhamRwt commented Jan 8, 2024 •

edited