Skip to content

Conversation

@javiertuya
Copy link
Collaborator

@javiertuya javiertuya commented May 19, 2025

Fixed Which Issue?

PR Branch
javiertuya:714-multiple-jdks

Motivation and Context

After merging #713 the build supports Java versions from 8 to 23, but future changes could break compatibility if no tested in CI. This PR includes:

  • Use a matrix strategy to run the workflow on all LTS JDK versions (8,11,17,21) and the latest (23)
  • Add downloadable junit test reports
  • Workflow updates (java distribution, actions, docker compose, maven cache)

Checklist:

  • New Unit tests were added

    • Covered in existing Unit tests
  • Integration tests were added

    • Covered in existing Integration tests
  • Test names are meaningful

  • Feature manually tested and outcome is successful

  • PR doesn't break any of the earlier features for end users

    • WARNING! This might break one or more earlier earlier features, hence left a comment tagging all reviewrs
  • PR doesn't break the HTML report features directly

    • Yes! I've manually run it locally and seen the HTML reports are generated perfectly fine
    • Yes! I've opened the generated HTML reports from the /target folder and they look fine
  • PR doesn't break any HTML report features indirectly

    • I have not added or amended any dependencies in this PR
    • I have double checked, the new dependency added or removed has not affected the report generation indirectly
    • Yes! I've seen the Sample report screenshots here, and HTML report of the current PR looks simillar.
  • Branch build passed in CI

  • No 'package.*' in the imports

  • Relevant DOcumentation page added or updated with clear instructions and examples for the end user

    • Not applicable. This was only a code refactor change, no functional or behaviourial changes were introduced
  • Http test added to http-testing module(if applicable) ?

    • Not applicable. The changes did not affect HTTP automation flow
  • Kafka test added to kafka-testing module(if applicable) ?

    • Not applicable. The changes did not affect Kafka automation flow

@javiertuya javiertuya marked this pull request as ready for review May 19, 2025 15:22
@javiertuya javiertuya requested a review from nirmalchandra May 19, 2025 15:23
@javiertuya
Copy link
Collaborator Author

@authorjapps Done. When reviewing the workflow I have detected some points where it can be improved. Please, let me know if you want to add any of them:

  • Use -ntp argument to run the maven command. This will remove thousands unnecesary log lines that display the maven dependency download progress
  • Set-up the maven cache in actions/setup-java, sometimes, this achieves time improvements
  • Replace the java adopt distribution by temurin, it is more up to date and the GitHub recommendation
  • Check versions of the actions that are used in the workflow and update to the latest ones
  • Update docker compose to V2: AFIK, the command docker-compose is V1, docker compose (without dash) is V2 and it does not need to be installed in the runner

@authorjapps
Copy link
Owner

@authorjapps Done. When reviewing the workflow I have detected some points where it can be improved. Please, let me know if you want to add any of them:

  • Use -ntp argument to run the maven command. This will remove thousands unnecesary log lines that display the maven dependency download progress
  • Set-up the maven cache in actions/setup-java, sometimes, this achieves time improvements
  • Replace the java adopt distribution by temurin, it is more up to date and the GitHub recommendation
  • Check versions of the actions that are used in the workflow and update to the latest ones
  • Update docker compose to V2: AFIK, the command docker-compose is V1, docker compose (without dash) is V2 and it does not need to be installed in the runner

@javiertuya , All sounds great updates and make the build process would be much more meaningful.
So please proceed with the changes in the PR.

Happy to review when ready!

@javiertuya
Copy link
Collaborator Author

Happy to review when ready!

@authorjapps Done. There is a small improvement in the overall time (times seem to be below 10s), mainly due to the container spin-up, and the test step log size is 8K lines shorter.

@authorjapps
Copy link
Owner

Approved 🟢 @javiertuya , you can merge anytime you get chance.

@javiertuya
Copy link
Collaborator Author

Approved 🟢 @javiertuya , you can merge anytime you get chance.

@authorjapps I can't merge because the merge button is disabled. The cause (I guess) is the branch protection rule configuration for master: Currently, in the "Require status checks to pass before merging" there is a single requirement for the "build" status check. But now, the build status does not exist, there is a status check for each of the Java version: build (8), build (11) ...

Could you please check Settings -> Branches -> (edit the master branch protection rule) -> Require status checks to pass before merging? Remove build and add build (8), build (11), build (17), build (21), build (23),

@authorjapps
Copy link
Owner

yeah, just seen it @javiertuya , but not sure which rule is blocking it, it's disable for me as well. I have given a rerun, just to rule out a fresh run.

image

@authorjapps
Copy link
Owner

Approved 🟢 @javiertuya , you can merge anytime you get chance.

@authorjapps I can't merge because the merge button is disabled. The cause (I guess) is the branch protection rule configuration for master: Currently, in the "Require status checks to pass before merging" there is a single requirement for the "build" status check. But now, the build status does not exist, there is a status check for each of the Java version: build (8), build (11) ...

Could you please check Settings -> Branches -> (edit the master branch protection rule) -> Require status checks to pass before merging? Remove build and add build (8), build (11), build (17), build (21), build (23),

Fixed.

@javiertuya , Can you check if the "merge PR" Green 🟢 button is enabled for you ?

@javiertuya
Copy link
Collaborator Author

@javiertuya , Can you check if the "merge PR" Green 🟢 button is enabled for you ?

@authorjapps Yes, it is fine. Merging now

@javiertuya javiertuya merged commit 6cbf37d into authorjapps:master Jun 3, 2025
15 checks passed
@javiertuya javiertuya deleted the 714-multiple-jdks branch June 3, 2025 16:04
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.

[CI] Update the action script to build for higher JDK versions

3 participants