#7988-added labels and annotation keys into api module

[pratikkolkar]

Type of change

Select the type of your PR

• Enhancement

Description

As per issue #7988. Made label and annotation keys available in api module by adding Labels and Annotations class under io.strimzi.api package.

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]

Thanks for the PR. The new classes look good. But we do not want to keep these annotations / labels twice. So you would also need to remove the original ones and in the places they were used use these instead.

[ppatierno]

There are no utility methods in this class, or?

[pratikkolkar]

@ppatierno - You are right no utility methods, I will remove that from comment lines.

[pratikkolkar]

Thanks for the PR. The new classes look good. But we do not want to keep these annotations / labels twice. So you would also need to remove the original ones and in the places they were used use these instead.

@scholzj - I have a suggestion instead of using the labels/annotations from new class in all the places. Can't we extend the Labels and Annotations class from operator-common module with the new class, the parent class(from api module) will be having all the labels which we moved and the child class(from operator-common module) can inherit them , this way I believe we can avoid doing the changes to all the places where it is used and duplicates will be handled as well. Let me know your point of view on this.

[scholzj]

I think that would work, yes. It would also cause less confusion in the imports in places where both can be used. Does that sound reasonable to you as well @ppatierno?

[ppatierno]

Yea I am fine with it, thanks @pratikkolkar !

[scholzj]

I think the code changes look good. But the build failed because of some unused imports:

[ERROR] /home/vsts/work/1/s/cluster-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/ManualPodCleaner.java:18:8: Unused import - io.strimzi.operator.common.operator.resource.AbstractScalableNamespacedResourceOperator. [UnusedImports]
[ERROR] /home/vsts/work/1/s/cluster-operator/src/test/java/io/strimzi/operator/cluster/operator/assembly/ManualPodCleanerTest.java:19:8: Unused import - io.strimzi.operator.common.operator.resource.AbstractScalableNamespacedResourceOperator. [UnusedImports]

So you will need to remove those.

[scholzj]

Hmm, looks like Spotbugs does not like this:

[ERROR] High: The class name io.strimzi.operator.common.Annotations shadows the simple name of the superclass io.strimzi.api.Annotations [io.strimzi.operator.common.Annotations] At Annotations.java:[lines 24-288] NM_SAME_SIMPLE_NAME_AS_SUPERCLASS
[ERROR] High: The class name io.strimzi.operator.common.model.Labels shadows the simple name of the superclass io.strimzi.api.Labels [io.strimzi.operator.common.model.Labels] At Labels.java:[lines 108-501] NM_SAME_SIMPLE_NAME_AS_SUPERCLASS

I'm sure we can suppress it. But I wonder if it has a point. Should we name the classes in the api module somehow differently? ResourceAnnotations and ResourceLabels? Any thoughts @tombentley & @ppatierno?

[ppatierno]

I'm sure we can suppress it. But I wonder if it has a point. Should we name the classes in the api module somehow differently? ResourceAnnotations and ResourceLabels? Any thoughts @tombentley & @ppatierno?

In general I don't like having classes linked by an inheritance relationship with same name (even if living in different packages). I would do what you are suggesting.

[pratikkolkar]

I have renamed the classes as suggested. But getting this Import control error in operator-common module.

Is it feasible to replace <allow pkg="io.strimzi.api.annotations" /> and <allow pkg="io.strimzi.api.kafka" /> with <allow pkg="io.strimzi.api" /> in import-control.xml ?

[scholzj]

I would specifically add the two classes:

        <allow class="io.strimzi.api.ResourceLabels" />
        <allow class="io.strimzi.api.ResourceAnotations" />

[scholzj]

Thanks. This LGTM now.

[scholzj]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ppatierno]

LGTM

[scholzj]

Thanks for the PR @pratikkolkar ... Nice work!