-
Notifications
You must be signed in to change notification settings - Fork 3
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
Upgrade to Play v2.9, Scala 2.13, Java 11 #349
Conversation
9cb50a2
to
e5338de
Compare
The project also requires Java version `1.8.0.232` (or lower). The easiest way to set this is with `jenv`: | ||
- `brew install jenv` | ||
- Download and install the appopriate Java version: [jdk8u232-b09](https://adoptopenjdk.net/archive.html) | ||
- `jenv add /Library/Java/JavaVirtualMachines/adoptopenjdk-8.jdk/Contents/Home/` | ||
The project also requires Java 11. The [easiest](https://docs.google.com/document/d/1ZR-YnaXCT5_gLVmTCeGs0mWd3KPaAozPjQK8uUzHZ9w/edit#heading=h.kgqqi53p3ltt) | ||
way to install this is with `asdf install`, which will install the version of `java` specified in our | ||
[.tool-versions](.tool-versions) file: |
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.
I'm dropping jenv
here in favour of asdf
, as that's what DevX currently recommend. Consequently, .java-version
becomes .tool-versions
and changes format slightly.
project/Dependencies.scala
Outdated
@@ -3,7 +3,7 @@ import sbt._ | |||
|
|||
object Dependencies { | |||
lazy val awsVersion = "1.11.678" | |||
lazy val atomLibVersion = "1.4.0" | |||
lazy val atomLibVersion = "2.0.0-PREVIEW.fix-tests-under-java-17.2024-01-18T1039.b4d55b3d" |
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.
This is a preview build provided of guardian/atom-maker#94, which adds support for Play 2.9 - once that PR is merged, we should update this PR to use the final 2.0.0 version, but in the mean time this is good for trying out the changes.
e5338de
to
39ada95
Compare
- name: Cache SBT | ||
uses: actions/cache@v2 | ||
with: | ||
path: | | ||
~/.ivy2/cache | ||
~/.sbt | ||
key: ${{ runner.os }}-sbt-${{ hashFiles('**/build.sbt') }} |
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 setup-java
GHA now has built-in support for sbt
caching (added with actions/setup-java#302) so we don't need to try to provide our own definition for caching sbt resources.
These upgrades were prompted by guardian/atom-maker#94, but they're all a good idea anyway: * Play: 2.8 → 2.9 * Scala: 2.12 → 2.13 * Java: 8 → 11
39ada95
to
6716668
Compare
Recipe: editorial-tools-focal-java8-ARM-WITH-cdk-base | ||
Recipe: editorial-tools-focal-java11-ARM-WITH-cdk-base |
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.
Here we're changing AMIgo recipes, from:
editorial-tools-focal-java8-ARM-WITH-cdk-base
...to:
editorial-tools-focal-java11-ARM-WITH-cdk-base
The newer recipe already exists, and is in use for Composer, editorial feeds, etc.
Thanks @rebecca-thompson for checking atom-workshop in CODE, and reporting the configuration issue in the logs:
Looking at this config: atom-workshop/conf/application.conf Line 5 in 4e25b67
...it looks like the latest versions of Play don't tolerate such a short password (probably due to playframework/playframework#10726). At the Guardian we've used https://github.com/guardian/play-secret-rotation to address this issue securely in the past! |
https://www.playframework.com/documentation/2.9.x/ApplicationSecret Note that as atom-workshop currently doesn't _use_ the PLAY_SESSION cookie for anything (mainly because it uses https://github.com/guardian/pan-domain-authentication for authentication) it's probably okay for play.http.secret.key to _not_ be secret (which it isn't, being in this public repo) - but it _does_ need to be long enough to satisfy the Playframework and JJWT: playframework/playframework#10726 If atom-workshop ever _does_ use the PLAY_SESSION cookie, it should adopt https://github.com/guardian/play-secret-rotation to stay secure.
Prompted by this error message in the logs: application.conf @ file:/usr/share/atom-workshop/conf/application.conf: 9: parsers.text.maxLength is deprecated, use play.http.parser.maxMemoryBuffer instead Apparently the parsers.text.maxLength property has been deprecated since Play 2.4 and playframework/playframework#4093 https://www.playframework.com/documentation/2.9.x/ScalaBodyParsers#Max-content-length In atom-workshop, the increased size was set to allow bigger chart atoms: #272
Prompted by this error in the logs: Jan 26, 2024 @ 12:06:34.865 12:06:32,373 |-ERROR in ch.qos.logback.core.rolling.RollingFileAppender[FILE] - openFile(application.home_IS_UNDEFINED/logs/application.log,true) call failed. java.io.FileNotFoundException: application.home_IS_UNDEFINED/logs/application.log (No such file or directory) See also: * https://stackoverflow.com/a/47331903 * https://github.com/guardian/ophan/blob/2ecb80da40915ce097e4a1de0e29b352b396323e/dashboard/conf/logback.xml#L16-L19
parsers.text.maxLength=200kB | ||
play.http.parser.maxMemoryBuffer=200K |
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.
Prompted by this error message in the logs:
application.conf @ file:/usr/share/atom-workshop/conf/application.conf: 9: parsers.text.maxLength is deprecated, use play.http.parser.maxMemoryBuffer instead
Apparently the parsers.text.maxLength property
has been deprecated since Play 2.4 and playframework/playframework#4093 - play.http.parser.maxMemoryBuffer
is preferred.
In atom-workshop, the increased size was set to allow bigger chart atoms: #272
Prompted by this in the logs: Jan 26, 2024 @ 16:44:08.273 java.lang.NoSuchMethodError: 'void play.api.mvc.DiscardingCookie.<init>(java.lang.String, java.lang.String, scala.Option, boolean)' Jan 26, 2024 @ 16:44:08.273 at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020) Jan 26, 2024 @ 16:44:08.273 at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656 Jan 26, 2024 @ 16:44:08.271 at akka.dispatch.ForkJoinExecutorConfigurator$AkkaForkJoinTask.exec(ForkJoinExecutorConfigurator.scala:48) Jan 26, 2024 @ 16:44:08.271 at com.gu.pandomainauth.action.AuthActions.com$gu$pandomainauth$action$AuthActions$$discardCookies$(Actions.scala:102) I had forgotten to update the version of Panda used - we're upgrading to Play 2.9, so we need pan-domain-auth-play_2-9.
"com.gu" %% "fezziwig" % "1.2", | ||
"com.gu" %% "pan-domain-auth-play_2-8" % "1.2.0", | ||
"com.gu" %% "fezziwig" % "1.6", | ||
"com.gu" %% "pan-domain-auth-play_2-9" % "3.0.1", |
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.
I initially forgot to update this, but crucially, as we're updating to Play 2.9, we need to use pan-domain-auth-play_2-9
!
Otherwise we get a NoSuchMethodError
in the logs:
Jan 26, 2024 @ 16:44:08.273 java.lang.NoSuchMethodError: 'void play.api.mvc.DiscardingCookie.(java.lang.String, java.lang.String, scala.Option, boolean)'
Jan 26, 2024 @ 16:44:08.273 at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020)
Jan 26, 2024 @ 16:44:08.273 at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656
Jan 26, 2024 @ 16:44:08.271 at akka.dispatch.ForkJoinExecutorConfigurator$AkkaForkJoinTask.exec(ForkJoinExecutorConfigurator.scala:48)
Jan 26, 2024 @ 16:44:08.271 at com.gu.pandomainauth.action.AuthActions.com$gu$pandomainauth$action$AuthActions$$discardCookies$(Actions.scala:102)
<file>${application.home}/logs/application.log</file> | ||
<file>logs/application.log</file> | ||
|
||
<rollingPolicy class="ch.qos.logback.core.rolling.FixedWindowRollingPolicy"> | ||
<fileNamePattern> | ||
${application.home}/logs/application.log.%i | ||
</fileNamePattern> | ||
<fileNamePattern>logs/application.log.%i</fileNamePattern> |
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.
This change was prompted by this error in the logs:
Jan 26, 2024 @ 12:06:34.865 12:06:32,373 |-ERROR in ch.qos.logback.core.rolling.RollingFileAppender[FILE] - openFile(application.home_IS_UNDEFINED/logs/application.log,true) call failed. java.io.FileNotFoundException: application.home_IS_UNDEFINED/logs/application.log (No such file or directory)
Whatever ${application.home}
was, it's not available anymore! With the updated settings (copied from Ophan) the error message is gone, and we still get log messages in the https://logs.gutools.co.uk/ ELK, where it matters.
See also https://stackoverflow.com/a/47331903, though I don't think we really needed the fallback.
I'm not sure why I set the CI Java version to 17 (other than I have started using that Java version elsewhere) - but as the AMI is only `editorial-tools-focal-java11-ARM-WITH-cdk-base`, when the server tried to run code that had been compiled with Java 17, it gave an UnsupportedClassVersionError like this in the logs: play.api.UnexpectedException: Unexpected exception[RuntimeException: java.lang.UnsupportedClassVersionError: controllers/routes has been compiled by a more recent version of the Java Runtime (class file version 61.0), this version of the Java Runtime only recognizes class file versions up to 55.0] at play.api.http.HttpErrorHandlerExceptions$.throwableToUsefulException(HttpErrorHandler.scala:384)
d39b421
to
6eee1b8
Compare
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.
Looks good - runs nicely on CODE and locally - not sure many ed tools devs use asdf but probably an improvement on jenv.
These upgrades were prompted by guardian/atom-maker#94, but they're all probably a good idea anyway:
Note that this PR is using a preview release of guardian/atom-maker#94, which should be merged before this PR.
As of commit d39b421, this PR has deployed successfully to CODE:
No
editorial-tools-platform
updates requiredWe've reviewed https://github.com/guardian/editorial-tools-platform/blob/main/cloudformation/composer-account/atom-workshop/atom-workshop.yml to check that we don't need to update a Java version there (ie we haven't forgotten it like we did with guardian/media-atom-maker#1149 ).