Skip to content

Conversation

@kbendick
Copy link
Contributor

This PR is trying to address this comment: #3501 (comment)

Many thanks to @openinx and @stevenzwu who noticed that the checkstyle wasn't running.

I recently had to fix something similar with Spark for the 0.12.1 release build, but I thought it was due to the version split up (and that somebody's PR get in without running tests for a rebase).

Removing quick=true causes checkstyle to run again. For CI, we should be running all checks (and can work to disable some if we don't need them).

@openinx suggested in this PR: #3509 that we remove the path ignores in java-ci.yaml. But I think that would negate the work of splitting up the tests and the speed built up that we have seen from that.

If we find this takes too long (as it does run checkstyle fot transitive deps), then we can make one single check run but keep the Java CI and associated tests standalone still as another option.

In either case, this PR will flesh out any existing checks that aren't presently passing as things have not been running. So if we decide to go the route @openinx suggested, this PR (and others like it) will still find any existing checkstyle error issues and give us a chance to fix them.

@github-actions github-actions bot added the INFRA label Nov 10, 2021
@kbendick kbendick force-pushed the disable-quick-build-flink branch 2 times, most recently from cea539f to 47bb048 Compare November 10, 2021 06:55
@kbendick kbendick force-pushed the disable-quick-build-flink branch from 47bb048 to d538096 Compare November 10, 2021 06:57
@kbendick
Copy link
Contributor Author

Closing this as I'm trying to add openinx as co-author but it's not picking up (maybe bc it's already opened).

@kbendick kbendick closed this Nov 10, 2021
@kbendick kbendick deleted the disable-quick-build-flink branch November 10, 2021 07:01
@openinx
Copy link
Member

openinx commented Nov 10, 2021

As for adding me as co-author or not, it's okay for me either. The key thing is having a consistent idea to fix this as soon as possible before we merge any PR with basic checkstyle issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants