Skip to content

Use JDK 17 based image for Server IT#14614

Merged
Praveen2112 merged 1 commit intotrinodb:masterfrom
Praveen2112:praveen/server_it_failure
Oct 13, 2022
Merged

Use JDK 17 based image for Server IT#14614
Praveen2112 merged 1 commit intotrinodb:masterfrom
Praveen2112:praveen/server_it_failure

Conversation

@Praveen2112
Copy link
Copy Markdown
Member

Description

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

Copy link
Copy Markdown
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

All other usages of the image are pinned to a version so not failing yet but will start failing on next docker-images update.

@nineinchnick
Copy link
Copy Markdown
Member

The failures in tests are caused by treating an empty impacted modules list as a signal to run all tests. The list is empty because the rpm module is excluded when running mvn validate.

@Praveen2112 can you also add these changes?

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index f02c356b6d5..fc074784184 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -408,7 +408,7 @@ jobs:
       - name: Maven validate
         run: |
           export MAVEN_OPTS="${MAVEN_INSTALL_OPTS}"
-          $RETRY $MAVEN validate ${MAVEN_FAST_INSTALL} ${MAVEN_GIB} -Dgib.logImpactedTo=gib-impacted.log -P disable-check-spi-dependencies -pl '!:trino-docs,!:trino-server,!:trino-server-rpm'
+          $RETRY $MAVEN validate ${MAVEN_FAST_INSTALL} ${MAVEN_GIB} -Dgib.logImpactedTo=gib-impacted.log -P disable-check-spi-dependencies -pl '!:trino-docs'
       - name: Set matrix
         id: set-matrix
         run: |
@@ -658,7 +658,7 @@ jobs:
       - name: Map impacted plugins to features
         run: |
           export MAVEN_OPTS="${MAVEN_INSTALL_OPTS}"
-          $MAVEN validate ${MAVEN_FAST_INSTALL} ${MAVEN_GIB} -Dgib.logImpactedTo=gib-impacted.log -pl '!:trino-docs,!:trino-server-rpm'
+          $MAVEN validate ${MAVEN_FAST_INSTALL} ${MAVEN_GIB} -Dgib.logImpactedTo=gib-impacted.log -pl '!:trino-docs'
           # GIB doesn't run on master, so make sure the file always exist
           touch gib-impacted.log
           testing/trino-plugin-reader/target/trino-plugin-reader-*-executable.jar -i gib-impacted.log -p core/trino-server/target/trino-server-*-hardlinks/plugin > impacted-features.log

@Praveen2112
Copy link
Copy Markdown
Member Author

Should it be as a commit - that we need to remove before merging ?

@nineinchnick
Copy link
Copy Markdown
Member

It can be a separate commit, before the one you have right now. It should be merged.

@nineinchnick
Copy link
Copy Markdown
Member

I was considering doing this in a separate PR but I'd have to duplicate your change in the rpm module anyway.

@Praveen2112 Praveen2112 force-pushed the praveen/server_it_failure branch from f24e7fa to f7351cd Compare October 13, 2022 09:33
@Praveen2112
Copy link
Copy Markdown
Member Author

@nineinchnick Applied it.

@Praveen2112 Praveen2112 force-pushed the praveen/server_it_failure branch 2 times, most recently from 790f08c to 4b05d79 Compare October 13, 2022 09:37
@Praveen2112 Praveen2112 force-pushed the praveen/server_it_failure branch from 4b05d79 to 2dc8bf4 Compare October 13, 2022 10:16
@Praveen2112
Copy link
Copy Markdown
Member Author

@nineinchnick I think we can handle them in a different PR.

@nineinchnick
Copy link
Copy Markdown
Member

I added a fix for the tests/exclusion issue in #12854 and tested it in nineinchnick#14

@Praveen2112
Copy link
Copy Markdown
Member Author

@nineinchnick Should we worry about the test failures here ?

@Praveen2112
Copy link
Copy Markdown
Member Author

Maven checks were successful.

@Praveen2112 Praveen2112 merged commit 7a34c24 into trinodb:master Oct 13, 2022
@findepi
Copy link
Copy Markdown
Member

findepi commented Oct 13, 2022

Use JDK 17 based image for Server IT

What changed? I guess Java 17 was used even before the change, just with a different image, otherwise the container wouldn't start at all, would it?

@hashhar
Copy link
Copy Markdown
Member

hashhar commented Oct 13, 2022

The change was that centos-oj11 had both JDK 11 and 17 installed. this was recently fixed in trinodb/docker-images@7e3b08a and a release was made.

this test didn't pin the version so got broken.

Other tests will also break at next update of docker-images.

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

Development

Successfully merging this pull request may close these issues.

4 participants