Skip to content

Build: Enable engine's checkstyle and error-prone check in Github CI #3509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rdblue merged 3 commits into from
Nov 14, 2021
+0 −12
Merged

Build: Enable engine's checkstyle and error-prone check in Github CI #3509

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
333bdc5
Build: Enable checkstyle and error-prone check in Github CI
openinx Nov 9, 2021
c5e981e
Enable Java check for engines.
openinx Nov 9, 2021
34038ad
Revert the unrequired change
openinx Nov 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions .github/workflows/java-ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,6 @@ on:
- '.github/workflows/cancel-duplicate-workflow-runs.yml'
- '.gitignore'
- 'dev/**'
- 'mr/**'
- 'hive3/**'
- 'hive3-orc-bundle/**'
- 'hive-runtime/**'
- 'spark/**'
- 'flink/**'
Comment on lines -31 to -36
Copy link
Contributor

@kbendick kbendick Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this will solve your problem, but it might also lead to running java-ci a lot more than is necessary, as it does more than just checkstyle.

Am I correct in thinking that we're just trying to ensure checkstyle runs over changed files? Is there a way we can avoid running the additional 6-11 minute action that makes up all of java-ci for all of these paths if we only need a subset of it?

Or maybe we can run checkstyle in its own workflow to avoid having to run it in every single workflow.

Copy link
Member Author

@openinx openinx Nov 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my view, the overall checkstyle check is necessary. I think the problem is: removing all those ignored engine paths will trigger the extra iceberg-core unit tests check. Because if any engine related changes happen , it will always run the iceberg-core unit tests (even we don't ) I think.

Copy link
Contributor

@rdblue rdblue Nov 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @openinx. This common rule is the one that should be running the checks. This is to avoid making all of the CI builds more expensive because checkstyle and errorprone take a long time to run.

One thing that we still need to do after this is enable all Flink, Spark, and Hive versions in the job config, not just the default ones.

- 'python/**'
- 'python_legacy/**'
- 'site/**'
Expand All @@ -45,12 +39,6 @@ on:
- '.github/workflows/hive-ci.yml'
- '.gitignore'
- 'dev/**'
- 'mr/**'
- 'hive3/**'
- 'hive3-orc-bundle/**'
- 'hive-runtime/**'
- 'spark/**'
- 'flink/**'
- 'python/**'
- 'python_legacy/**'
- 'site/**'
Expand Down