KAFKA-12770: CheckStyle team needs this feature (in order to include Kafka into their regression suite) #10967

dejan2609 · 2021-07-03T18:45:53Z

@ijuma please review.

Rationale /related comment: #10656 (comment)

Description: checkstyleVersion build option is introduced (for overriding CheckStyle project-defined dependency version).

Note: previous PR #10698 (that contains identical commit) is closed: it somehow slipped through the cracks, so I opted to open a new one.

showuon · 2021-07-03T23:54:36Z

You don't actually have to create a new one. :)

dejan2609 · 2021-07-20T14:59:55Z

@ijuma could you please review this PR ?

ijuma · 2021-07-20T15:03:21Z

Hi, I think I'll be able to get to this next week.

dejan2609 · 2021-09-07T21:54:53Z

Adding a comment (just to bring this PR to the surface).

dejan2609 · 2021-10-24T10:29:47Z

@ijuma kindly asking for a review.

dejan2609 · 2022-01-20T18:47:35Z

@ijuma just trying to get some attention for this.

dejan2609 · 2022-03-22T19:51:00Z

Note: rebased and force-pushed (in order to resolve conflicts).

dejan2609 · 2023-02-26T21:23:53Z

@ijuma you may want to review this oldtimer💡

github-actions · 2024-12-01T03:48:04Z

This PR is being marked as stale since it has not had any activity in 90 days. If you
would like to keep this PR alive, please leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.

If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact).

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

chia7712

@dejan2609 thanks for this patch.

chia7712 · 2024-12-01T11:54:52Z

build.gradle

  ]
+
+  // See README.md for this build option details and example of usage
+  checkstyleVersion = project.hasProperty('checkstyleVersion') ? checkstyleVersion : versions.checkstyle


Maybe we should configure the version in dependencies.gradle, similar to how the Scala version is configured? This would ensure consistency and better version management.

Hi @chia7712 and thanks for stopping by ! Please allow me to add some more details below.

Prologue (relevant comment by @romani from CheksTyle team): #10656 (comment)

This (now fairly dusty) PR was created in order to provide CheckStyle team with some handy way to include Kafka project into CheckStyle regression suite. As time went by PR slipped through the cracks (but I decided to stick with it and hence we are having a discussion now 😃).

So, if this PR gets into Kafka trunk CheckStyle team will be able to perform following sequence on their end, i.e. on their CI server (note: X.yz stands for any specific CheckStyle version):

git clone git checkout trunk ./gradlew check -PcheckstyleVersion=X.yz

As for a place where this change should reside (dependencies.gradle vs. common build options) I am open for a discussion.
Current proposal is aligned with suggestion made by @ijuma here: #10656 (comment)

is there a way to run only chckstyle task ?
I think that "check" will run all validations.

What version of checkstyle is used now?

Current proposal is aligned with suggestion made by @ijuma here: #10656 (comment)

yes, it is good to use ./gradlew check -PcheckstyleVersion=X.yz but it seems @ijuma 's comment is not related to the implementation. Personally, versions.checkstyle is the version used by whole project, and hence I prefer to handle the custom version in the dependencies.gradle

Hi @romani !

Currently Kafka is using Checkstyle version 8.36.2, but today @m1a2st created this (pretty impressive) PR that bumps Checkstyle to 10.20.1: https://github.com/apache/kafka/pull/17999/files#diff-02b139e755ab0e27e0b0a6f9845843ef189116955cc340c49a4022587dbb2b52

As for a checkstyle-only Gradle task: I was under impression that ./gradlew check is performing only Checkstyle checks, but after some experimenting I see that it not the case (maybe I can deal with that in a separate PR).

Personally, versions.checkstyle is the version used by whole project, and hence I prefer to handle the custom version in the dependencies.gradle

@chia7712 makes sense, dependencies.gradle it is then.

@chia7712 pushed, let me know what you think (eventually I will squash commits into one, of course).

@romani we can continue our discussion related to fine-grained Checkstyle task(s); I will make a mental note to my self to open some tiket or draft some push request.

We can improve on fine-grained later. Let's merge something that works.

Sure @romani, got that.

@chia7712 solution is squashed into one commit.

dejan2609 · 2024-12-01T15:01:51Z

Just a few of tests are failing (but they are unrelated/flaky).

chia7712

LGTM

…iding CheckStyle project-defined dependency version) rationale/notes: * useful for experimenting and regression testing * requested by a CheckStyle team so they can add Kafka into their regression suite

dejan2609 · 2024-12-01T20:54:59Z

@chia7712 squashed and force-pushed.

Note: few unrelated and flaky tests failed (I guess we are good to go).

dejan2609 · 2024-12-02T09:07:58Z

@chia7712 thanks for an approval.
Can you merge this one into the trunk ? Or we need to wait for another (pending) review ?

dejan2609 · 2024-12-02T13:27:17Z

Thank you @chia7712, much obliged !

dejan2609 · 2024-12-05T08:07:36Z

@romani now that trunk branch CheckStyle is upgraded to a latest and greatest version (#17999 🚀) you are free to add Kafka project into your regression suite with:

./gradlew checkstyleMain checkstyleTest

Note: given a fact that CheckStyle deals with coding standards you could opt to skip Kafka's tests execution on your end with -x test switch (in order to cut build time).

…ding CheckStyle project-defined dependency version) (apache#10967) Reviewers: Ken Huang <[email protected]>, Luke Chen <[email protected]>, Chia-Ping Tsai <[email protected]>

dejan2609 mentioned this pull request Jul 3, 2021

KAFKA-12770: introduce checkstyleVersion build option (for overriding CheckStyle project-defined dependency version) #10698

Closed

showuon approved these changes Jul 3, 2021

View reviewed changes

dejan2609 force-pushed the checkstyleRegession branch from 0079e1f to 1872cb3 Compare March 22, 2022 19:49

dejan2609 force-pushed the checkstyleRegession branch from 1872cb3 to 6c26c22 Compare April 13, 2022 18:57

dejan2609 force-pushed the checkstyleRegession branch from 6c26c22 to eee8c62 Compare June 25, 2022 19:39

dejan2609 force-pushed the checkstyleRegession branch from eee8c62 to 378008e Compare July 16, 2022 06:03

dejan2609 force-pushed the checkstyleRegession branch from 378008e to 202bff8 Compare February 26, 2023 21:21

divijvaidya requested a review from ijuma May 24, 2023 10:46

dejan2609 force-pushed the checkstyleRegession branch from 202bff8 to b9bfbcd Compare September 16, 2023 11:14

dejan2609 force-pushed the checkstyleRegession branch from b9bfbcd to 1c3afde Compare June 12, 2024 09:34

dejan2609 force-pushed the checkstyleRegession branch from 1c3afde to 0bcffb1 Compare August 9, 2024 12:40

github-actions bot added the stale Stale PRs label Dec 1, 2024

dejan2609 force-pushed the checkstyleRegession branch from 0bcffb1 to acdcc49 Compare December 1, 2024 11:26

github-actions bot added build Gradle build or GitHub Actions small Small PRs labels Dec 1, 2024

dejan2609 mentioned this pull request Dec 1, 2024

KAFKA-18050: Upgrade the checkstyle version to 10.20.2 #17999

Merged

3 tasks

chia7712 added the ci-approved label Dec 1, 2024

chia7712 reviewed Dec 1, 2024

View reviewed changes

chia7712 approved these changes Dec 1, 2024

View reviewed changes

KAFKA-12770: introduce checkstyleVersion gradle property (for overr…

7a867ee

…iding CheckStyle project-defined dependency version) rationale/notes: * useful for experimenting and regression testing * requested by a CheckStyle team so they can add Kafka into their regression suite

dejan2609 force-pushed the checkstyleRegession branch from 003ce39 to 7a867ee Compare December 1, 2024 20:52

m1a2st approved these changes Dec 2, 2024

View reviewed changes

chia7712 approved these changes Dec 2, 2024

View reviewed changes

chia7712 merged commit f9215da into apache:trunk Dec 2, 2024
7 of 8 checks passed

dejan2609 deleted the checkstyleRegession branch December 2, 2024 13:29

dejan2609 mentioned this pull request Dec 5, 2024

Add Kakfa project into Checkstyle regression suite checkstyle/checkstyle#16003

Closed

KAFKA-12770: CheckStyle team needs this feature (in order to include Kafka into their regression suite) #10967

KAFKA-12770: CheckStyle team needs this feature (in order to include Kafka into their regression suite) #10967

Uh oh!

Conversation

dejan2609 commented Jul 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

showuon commented Jul 3, 2021

Uh oh!

dejan2609 commented Jul 20, 2021

Uh oh!

ijuma commented Jul 20, 2021

Uh oh!

dejan2609 commented Sep 7, 2021

Uh oh!

dejan2609 commented Oct 24, 2021

Uh oh!

dejan2609 commented Jan 20, 2022

Uh oh!

dejan2609 commented Mar 22, 2022

Uh oh!

dejan2609 commented Feb 26, 2023

Uh oh!

github-actions bot commented Dec 1, 2024

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dejan2609 Dec 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dejan2609 commented Dec 1, 2024

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

dejan2609 commented Dec 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dejan2609 commented Dec 2, 2024

Uh oh!

Uh oh!

dejan2609 commented Dec 2, 2024

Uh oh!

dejan2609 commented Dec 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dejan2609 commented Jul 3, 2021 •

edited

Loading

dejan2609 Dec 1, 2024 •

edited

Loading

dejan2609 commented Dec 1, 2024 •

edited

Loading