Skip to content
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.

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

[FLINK-29436][build] Upgrade spotless for running on JDK 17 #20911

Merged
merged 5 commits into from
Sep 29, 2022

Conversation

tisonkun
Copy link
Member

This blocker is fixed by: diffplug/spotless#1224 and diffplug/spotless#1228.

pom.xml Show resolved Hide resolved
@flinkbot
Copy link
Collaborator

flinkbot commented Sep 28, 2022

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@zentol
Copy link
Contributor

zentol commented Sep 28, 2022

Could there be a bug in that spotless version that the skip parameter is being ignored?

@tisonkun
Copy link
Member Author

@zentol with this change I tested JDK 17 can work with spotless:

gh pr checkout 20911
mvn -nsu spotless:apply

... while it generates diff on:

  • flink-connectors/flink-sql-connector-hive-3.1.2/src/main/java/org/apache/hadoop/hive/conf/HiveConf.java
  • flink-connectors/flink-sql-connector-hive-3.1.2/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java

And I pushed a second commit for the change.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Member Author

tisonkun commented Sep 28, 2022

It seems the version bump cause this configure not work, I will try to find a reasonable followup:

			<!-- disable spotless since we need copy hive code -->
			<plugin>
				<groupId>com.diffplug.spotless</groupId>
				<artifactId>spotless-maven-plugin</artifactId>
				<configuration>
					<skip>true</skip>
				</configuration>
			</plugin>

Related issue: diffplug/spotless#1227

Signed-off-by: tison <wander4096@gmail.com>
This reverts commit 16c03fb.
@tisonkun
Copy link
Member Author

Pushed a workaround on the plugin configs. I'm submitting a patch for upstream to avoid such a workaround.

Copy link
Contributor

@zentol zentol left a comment

Choose a reason for hiding this comment

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

+1 % CI passing

@zentol zentol self-assigned this Sep 28, 2022
@tisonkun
Copy link
Member Author

@zentol thanks for your reviewing.

I've submitted a patch to the upstream diffplug/spotless#1353.

Supposed 1) we're not in a hurry on this change 2) it is not easily to conflict with other patches 3) upstream uses auto-release after a patch is merged. I suggest after CI passes we wait a few days (e.g., after the Chinese National Day holiday I'll come back on Oct. 8th) to try to eliminate the workaround in this patch.

@zentol
Copy link
Contributor

zentol commented Sep 28, 2022

Sure, let's wait for the release.

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Member Author

Upgrade to 2.27.1 and eliminate the workaround. Wait for the new CI report and merge.

@tisonkun
Copy link
Member Author

@flinkbot run azure

@tisonkun
Copy link
Member Author

Although test fails should be unrelated, IIRC someone said you should get CI passed to merge a patch, lol. Rerun.

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

Successfully merging this pull request may close these issues.

3 participants