Skip to content

Fix the JDK8 github action + make local build developer friendly + fix coverage#1255

Merged
samikshya-db merged 7 commits intodatabricks:mainfrom
samikshya-db:samikshya-chand_data/fixBrokenPackagingTests
Mar 10, 2026
Merged

Fix the JDK8 github action + make local build developer friendly + fix coverage#1255
samikshya-db merged 7 commits intodatabricks:mainfrom
samikshya-db:samikshya-chand_data/fixBrokenPackagingTests

Conversation

@samikshya-db
Copy link
Collaborator

@samikshya-db samikshya-db commented Mar 6, 2026

Description

  1. The JDK8 GH action is broken, this PR fixes it.
  2. This PR also adds a maven profile that does compile, run unit tests, and package and nothing else (similar to our old mvn clean test)
    1. What -Plocal skips:
    • NVD/dependency-check scan
    • Arrow patch tests (org.apache.arrow.memory.*)
    • Integration & fake service tests
    • Proxy, SSL, and logging tests

Testing

  • Run mvn -pl jdbc-core -Plocal clean test
  • For packaging, mvn -pl jdbc-core,assembly-uber install -Plocal

Additional Notes to the Reviewer

Note : arrow specific tests are not relevant in jdk8 branch as it is specific for JDK16+

NO_CHANGELOG=true

.flattened-pom.xml files generated by flatten-maven-plugin were
accidentally committed. Remove them and add to .gitignore to prevent
recurrence.

Signed-off-by: samikshya-chand_data <samikshya.chand@databricks.com>
@samikshya-db samikshya-db requested a review from tejassp-db March 6, 2026 11:13
Copy link
Collaborator

@tejassp-db tejassp-db left a comment

Choose a reason for hiding this comment

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

In prCheck.yml the following line is sufficient mvn -pl jdbc-core clean test -Dgroups='!Jvm17PlusAndArrowToNioReflectionDisabled' jacoco:report. If it works here then there is no need to add a profile for local. Why is it not working here?

@samikshya-db
Copy link
Collaborator Author

samikshya-db commented Mar 6, 2026

@tejassp-db , For your comment
If it works here then there is no need to add a profile for local
No it does not work on local if the repo secrets are not exported locally. I do not want to run e2e locally that is. Moreover, the NVD checks take a long time if cache-key is not present.

In short - the local profile is just a convenience shortcut for developers here

@samikshya-db samikshya-db requested a review from tejassp-db March 6, 2026 12:06
samikshya-db and others added 3 commits March 6, 2026 17:36
Keep in mind that jdk8 branch is 3 months old
All Arrow tests are tagged with Jvm17PlusAndArrowToNioReflectionDisabled
and run only via dedicated CI steps, not the general coverage run.
Excluding them from the JaCoCo report prevents a false coverage drop.

Signed-off-by: samikshya-chand_data <samikshya.chand@databricks.com>
@samikshya-db samikshya-db changed the title Fix the JDK8 github action + make local build developer friendly Fix the JDK8 github action + make local build developer friendly + fix coverage Mar 6, 2026
restore-keys: ${{ runner.os }}-jdk8-m2

- name: Run Unit Tests
run: mvn -pl jdbc-core clean test -Dgroups='!Jvm17PlusAndArrowToNioReflectionDisabled'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be using the local profile defined ind jdbc-core/pom.xml?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussed offline, the jdk-8 branch is 2 months old and it will not recognise the new command.
In case we happen to update the branch, the repo actions workflows will fail. In which case, we will have to update the workflow if we update the branch.

@tejassp-db tejassp-db self-requested a review March 10, 2026 04:44
Copy link
Collaborator

@tejassp-db tejassp-db left a comment

Choose a reason for hiding this comment

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

LGTM with the known caveat that this branch is behind master, and will be merged in the future.

@samikshya-db samikshya-db merged commit 93cf304 into databricks:main Mar 10, 2026
14 of 15 checks passed
@samikshya-db samikshya-db deleted the samikshya-chand_data/fixBrokenPackagingTests branch March 10, 2026 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants