-
Notifications
You must be signed in to change notification settings - Fork 98
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
SNOW-1269024 Installing multiple Java versions for test purposes #824
SNOW-1269024 Installing multiple Java versions for test purposes #824
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please see if @sfc-gh-japatel could take a look
@@ -18,9 +18,12 @@ jobs: | |||
- name: Checkout Code | |||
uses: actions/checkout@v2 | |||
- name: "Install Java 8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update the name as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to go with different approach - here only Java 8 is installed, hence the name remains the same.
Added JAVA distribution as it no longer dafaults to 'zulu' and is required parameter.
Added JAVA distribution as it no longer dafaults to 'zulu' and is required parameter.
Changed approach to only install second JAVA - the one required for tests, matrix for test-JAVA version
Add verification of java used by print to log
Aligned step naming to contain matrix value used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any plans for confluent end to end test?
@@ -14,13 +14,15 @@ jobs: | |||
matrix: | |||
apache_kafka_version: [ '2.5.1', '2.8.1', '3.2.1' ] | |||
snowflake_cloud: [ 'AWS', 'AZURE', 'GCS' ] | |||
java_test_version: [ '8', '11', '17' ] | |||
steps: | |||
- name: Checkout Code | |||
uses: actions/checkout@v2 | |||
- name: "Install Java 8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this step? as in we are installing java 8 here but using different versions below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the whole point of my change is to use two different versions of java - java 8 for building and unit testing but then 8, 11 and 17 as jre to run the app and verify it against given runtime environment - I added PRs description now as I switched it to "ready for review" so I hope it's more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 💪
…wflakedb#824) Building and unit testing process remains the same (Java8) but e2e testing is run on three different versions of JAVA: 8, 11 and 17. This means that there are 3 times more testing jobs, but they all run in parallel, hence total run time should remain ~the same.
Overview
SNOW-1269024 - adding verification on different versions of JRE
Building and unit testing process remains the same (Java8) but e2e testing is run on three different versions of JAVA: 8, 11 and 17. This means that there are 3 times more testing jobs, but they all run in parallel, hence total run time should remain ~the same.
Pre-review checklist
snowflake.ingestion.method
.Yes
- Added end to end and Unit Tests.No
- This is test only changenormal
Longer test runs