Skip to content
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

Merged
merged 7 commits into from Feb 14, 2024
Merged

Conversation

rtyley
Copy link
Member

@rtyley rtyley commented Jan 18, 2024

These upgrades were prompted by guardian/atom-maker#94, but they're all probably a good idea anyway:

  • Play: 2.8 → 2.9
  • Scala: 2.12 → 2.13
  • Java: 8 → 11

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:

image

No editorial-tools-platform updates required

We'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 ).

@rtyley rtyley force-pushed the upgrade-to-scala-v2.13 branch 4 times, most recently from 9cb50a2 to e5338de Compare January 18, 2024 11:36
Comment on lines -12 to +14
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:
Copy link
Member Author

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.

@@ -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"
Copy link
Member Author

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.

Comment on lines -31 to -37
- name: Cache SBT
uses: actions/cache@v2
with:
path: |
~/.ivy2/cache
~/.sbt
key: ${{ runner.os }}-sbt-${{ hashFiles('**/build.sbt') }}
Copy link
Member Author

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.

@rtyley rtyley changed the title Upgrade to Scala v2.13 Upgrade to Play v2.9, Scala 2.13, Java 11 Jan 18, 2024
@rtyley rtyley marked this pull request as ready for review January 18, 2024 11:56
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
Recipe: editorial-tools-focal-java8-ARM-WITH-cdk-base
Recipe: editorial-tools-focal-java11-ARM-WITH-cdk-base
Copy link
Member Author

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.

@rtyley
Copy link
Member Author

rtyley commented Jan 25, 2024

Thanks @rebecca-thompson for checking atom-workshop in CODE, and reporting the configuration issue in the logs:


Jan 25, 2024 @ 09:55:40.691 | The application secret has not been set, and we are in prod mode. Your application is not secure.Jan 25, 2024 @ 09:55:40.691 | To set the application secret, please read https://playframework.com/documentation/latest/ApplicationSecret

Looking at this config:

play.http.secret.key="changeme"

...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
Copy link
Member Author

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",
Copy link
Member Author

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)

Comment on lines -5 to +8
<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>
Copy link
Member Author

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)
Copy link
Contributor

@rhystmills rhystmills left a 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.

@rtyley rtyley merged commit 5ac7972 into main Feb 14, 2024
1 check passed
@rtyley rtyley deleted the upgrade-to-scala-v2.13 branch February 14, 2024 15:16
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.

None yet

3 participants