Fix checkstyle check

[im-konge]

Type of change

• Bugfix

Description

This PR fixes the checkstyle check after the #10527 - the [] brackets are not recognizable by the checkstyle plugin, which caused that all the errors were skipped (mainly in the tests modules folders/modules). That means -> if we remove some class and the import is unused, normally it would cause that we get the UnusedImport error from checkstyle. But because all of the errors were skipped, we got the successful build even when the unused import was there.

Also, some of the checks were wrongly named.

I'm changing the [] brackets to (), changing the check types to correct ones and also adding more checks that are hit during the build.

Checklist

• Make sure all tests pass

[scholzj]

Please explain what exactly is the problem because it clearly is passing on the CI.

[scholzj]

One more comment -> leaving aside that nothing seems to be failing ... you should not exclude all the checks. If you do Javadocs in any of these parts we don't check, you should still do them properly. They should just not be mandatory.

[im-konge]

Please explain what exactly is the problem because it clearly is passing on the CI.

I'm mentioning it in the description - because of the [] brackets, all errors (like unused imports) were ignored. It's passing in the CI, because it skips all the errors. Not just those which are written in the suppressions.xml. That means - if we remove something and we have unused import, the checkstyle will show that the build was successful. Which is wrong.

you should not exclude all the checks. If you do Javadocs in any of these parts we don't check, you should still do them properly. They should just not be mandatory.

Sorry, I'm not sure if I understand. I'm just skipping those errors that are related to Javadoc (and which you added in the previous PR). Those skips are related to the tests modules/folders.

[scholzj]

Please explain what exactly is the problem because it clearly is passing on the CI.

I'm mentioning it in the description - because of the [] brackets, all errors (like unused imports) were ignored. It's passing in the CI, because it skips all the errors. Not just those which are written in the suppressions.xml. That means - if we remove something and we have unused import, the checkstyle will show that the build was successful. Which is wrong.

Ok, I guess that is my mistake, sorry. But that is not what I understood from the description so maybe you should make it more clear.

you should not exclude all the checks. If you do Javadocs in any of these parts we don't check, you should still do them properly. They should just not be mandatory.

Sorry, I'm not sure if I understand. I'm just skipping those errors that are related to Javadoc (and which you added in the previous PR). Those skips are related to the tests modules/folders.

The point is that if you use Javadocs, you should still do them correctly. E.g. with the right style, with all prarameters etc. So you should not exclude all the checks. You should only exclude the checks that require the Javadocs for the public fields. I.e., if you don't use Javadocs, you should be fine. If you use them, use them correctly.

[im-konge]

Ok, I guess that is my mistake, sorry. But that is not what I understood from the description so maybe you should make it more clear.

Yeah sorry, I will make it clear in the description.

The point is that if you use Javadocs, you should still do them correctly. E.g. with the right style, with all prarameters etc. So you should not exclude all the checks. You should only exclude the checks that require the Javadocs for the public fields. I.e., if you don't use Javadocs, you should be fine. If you use them, use them correctly.

Okay, I will check that. Thanks

[im-konge]

@scholzj I've updated the checks (removed the skipped error types that I added) and the code based on the checkstyle errors I got. The description is updated as well. Could you please have another look? Thanks!

[scholzj]

Isn't it missing the systemtests module?

[im-konge]

Isn't it missing the systemtests module?

That's covered by the <!-- Skip Javadoc checks for tests--> part in the suppressions.xml. I guess it doesn't need another rule now or?

[scholzj]

Isn't it missing the systemtests module?

That's covered by the <!-- Skip Javadoc checks for tests--> part in the suppressions.xml. I guess it doesn't need another rule now or?

I don't think it should ... what about issues like these:

strimzi-kafka-operator/systemtest/src/test/java/io/strimzi/systemtest/kafka/KafkaST.java

Lines 1102 to 1122 in fd74272

	/**
	* @description This test verifies the functionality of resizing JBOD storage volumes on a Kafka cluster.
	* It checks that the system can handle volume size changes and performs a rolling update to apply these changes.
	*
	* @steps
	* 1. - Deploy a Kafka cluster with JBOD storage and initial volume sizes.
	* - Kafka cluster is operational.
	* 2. - Produce and consume messages continuously to simulate cluster activity.
	* - Message traffic is consistent.
	* 3. - Increase the size of one of the JBOD volumes.
	* - Volume size change is applied.
	* 4. - Verify that the updated volume size is reflected.
	* - PVC reflects the new size.
	* 5. - Ensure continuous message production and consumption are unaffected during the update process.
	* - Message flow continues without interruption.
	*
	* @usecase
	* - jbod
	* - volume-resize
	* - persistent-volume-claims
	*/

[im-konge]

I don't think it should ... what about issues like these:

That doesn't seem to be a problem during the build. It has a problem with @randomtag{}, where the checkstyle expects that {} will contain some tag pointing to something else. But without the curly brackets, it seems to work.

[scholzj]

I don't think it should ... what about issues like these:

That doesn't seem to be a problem during the build. It has a problem with @randomtag{}, where the checkstyle expects that {} will contain some tag pointing to something else. But without the curly brackets, it seems to work.

Wait, you fixed those. I somehow missed it in the diff.

[scholzj]

Thanks.

[im-konge]

/azp run build

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).