Conversation
20274e9 to
54276dd
Compare
scholzj
left a comment
There was a problem hiding this comment.
Just some quick notes. Thanks for looking into it.
install/000-Namespace.yaml
Outdated
| kind: Namespace | ||
| metadata: | ||
| name: strimzi-access-operator | ||
| name: myproject |
There was a problem hiding this comment.
These changes should not be here?
There was a problem hiding this comment.
Thanks for the review. Careless mistake. Fixed
.checkstyle/checkstyle.xml
Outdated
| <!-- Javadoc --> | ||
| <module name="JavadocStyle"> | ||
| <property name="checkEmptyJavadoc" value="true"/> | ||
| <property name="checkFirstSentence" value="false"/> | ||
| </module> |
There was a problem hiding this comment.
It makes sense to move Javadoc validation to Checkstyle. But you should likely mirror the rules from other repos. See for example here: https://github.com/strimzi/strimzi-kafka-bridge/pull/1067/changes
Assuming there are more changes it needs in the code, opening a separate PR might make sense (it is not dependent on Java 21 anyway).
There was a problem hiding this comment.
Reverted this. I will work on this in a new PR as suggested. Thanks
api/pom.xml
Outdated
| <maven.compiler.source>21</maven.compiler.source> | ||
| <maven.compiler.target>21</maven.compiler.target> |
There was a problem hiding this comment.
Do we really need to mirror this in all modules? Why isn't that only in the main pom.xml? The same for javadoc.fail.on.warnings now, when it is disabled everywhere? (assuming it is still needed at all)
There was a problem hiding this comment.
Removed from all but main pom.
Dockerfile
Outdated
|
|
||
| # Set JAVA_HOME env var | ||
| ENV JAVA_HOME=/usr/lib/jvm/jre-17 | ||
| ENV JAVA_HOME=/usr/lib/jvm/jre-21 |
There was a problem hiding this comment.
| ENV JAVA_HOME=/usr/lib/jvm/jre-21 | |
| ENV JAVA_HOME=/usr/lib/jvm/jre-${JAVA_VERSION} |
im-konge
left a comment
There was a problem hiding this comment.
One comment + I agree with the Jakub's comments there. Thanks for looking on this :)
scholzj
left a comment
There was a problem hiding this comment.
So, you disabled the Javadoc validation from the JDK. But you do not seem to replace it with the Checkstyle Javadoc validation as we do it in other projects. I think that is something that needs to be done here or in a separate PR before this one (whichever you prefer). But I do not think we should just drop the validation now and say that we fix it some time later.
development-docs/DEV_GUIDE.md
Outdated
| example, to build docker images that have the Java 21 JRE installed use `JAVA_VERSION=21 make docker_build`. If not | ||
| present, the container images will use Java **21** by default. |
There was a problem hiding this comment.
Same as above - does this really make sense given we support Java 21 only?
development-docs/DEV_GUIDE.md
Outdated
| it to the desired Java version. For example, for building with Java 21 you can use `export JAVA_VERSION_BUILD=21`. | ||
|
|
||
| > *Note*: Operator currently developed and tested with Java 17. | ||
| > *Note*: Operator currently developed and tested with Java 21. |
There was a problem hiding this comment.
I know you did not added this section. But given we basically support Java 21 only - what point does it have?
|
@OwenCorrigan76 regarding what Jakub mentioned, so replacing the Javadoc validation with the Checkstyle one, you could get some inspiration from what was done on the bridge here strimzi/strimzi-kafka-bridge#1067 (using the bridge as example because it's a project which is size-wise similar to kafka access operator). Let me know if you need any advice/help/insights to better understand what's needed. |
|
Thanks a mil for the comments guys. Will address today. |
Signed-off-by: ocorriga <ocorriga@redhat.com>
Signed-off-by: ocorriga <ocorriga@redhat.com>
Signed-off-by: ocorriga <ocorriga@redhat.com>
8d1e07e to
f0e05c6
Compare
Signed-off-by: ocorriga <ocorriga@redhat.com>
f0e05c6 to
6217d2c
Compare
|
@OwenCorrigan76 The point of the Javadoc changes is that we do want to have the Javadoc changes validated. So you should not be disabling it without having the replacement first. So you can:
|
Signed-off-by: ocorriga <ocorriga@redhat.com>
|
Thanks @scholzj. I think this should now be ok?? I've added a suppression for Systemtests and unit tests as is done in other repos. Is that ok? There is some other failure on the build around |
.checkstyle/suppressions.xml
Outdated
| <suppress checks=".*" | ||
| files="systemtest[/\\].*\.java"/> |
There was a problem hiding this comment.
This is not a good thing IMO.
By this, you will completely disable all the checks - including * imports, unused imports etc.
Did you saw this in some other repo? We have these checks for STs disabled only for the generated classes, as we are not able to do anything with them (this is for the operators repo).
I think it is definitely fine to have a supression for tests and system tests. But as mentioned by Lukas, It should be minimized. So probably Javadocs only. And f it needs something more than just the Javadocs, please share what it complains about and we can have a look. In general, we should aim to remove these suppressions. But we can leave the STs Javadocs for later. You can check the Operators suppressions to see what we suppress there: https://github.com/strimzi/strimzi-kafka-operator/blob/main/.checkstyle/suppressions.xml
It seems that there was some Red Hat RPM repo outage. Restarting the build seemed to have worked. |
Signed-off-by: ocorriga <ocorriga@redhat.com>
.checkstyle/suppressions.xml
Outdated
| <suppress checks=".*" | ||
| files="io[/\\]strimzi[/\\]kafka[/\\]access[/\\]model[/\\].*(Builder|Fluent|FluentImpl)\.java"/> | ||
| <suppress checks="JavadocMethod|JavadocType|JavadocVariable|MissingJavadocType|MissingJavadocMethod" | ||
| files="(systemtest|src[/\\]test[/\\]java)[/\\].*\.java"/> |
There was a problem hiding this comment.
| files="(systemtest|src[/\\]test[/\\]java)[/\\].*\.java"/> | |
| files="systemtest[/\\]src[/\\]test[/\\]java)[/\\].*\.java"/> |
I think it should be like this? Or is | there on purpose?
There was a problem hiding this comment.
and without .java, you don't need it there (based on https://github.com/strimzi/strimzi-kafka-operator/blob/main/.checkstyle/suppressions.xml#L49)
There was a problem hiding this comment.
Or if you are trying to suppress it for both STs and for regular tests, I think it should be done separately. Because this is a bit confusing for me.
Signed-off-by: ocorriga <ocorriga@redhat.com>
im-konge
left a comment
There was a problem hiding this comment.
I think this will work fine, LGTM.
This PR moves the Strimzi Kafka Access Operator repo to Java 21.