Skip to content

Upgrade to Play v2.9#1140

Merged
rhystmills merged 1 commit intomainfrom
upgrade-to-play-v2.9
Feb 14, 2024
Merged

Upgrade to Play v2.9#1140
rhystmills merged 1 commit intomainfrom
upgrade-to-play-v2.9

Conversation

@rtyley
Copy link
Copy Markdown
Member

@rtyley rtyley commented Jan 30, 2024

This upgrade from Play 2.8 → 2.9 was prompted by guardian/atom-maker#94, but it's a good idea anyway!

This PR was initially tried-out using a preview release of guardian/atom-maker#94 (which updates those libraries to compile against Play 2.9), but that has now been merged & released as v2.0.0, so we've updated this PR to use that final release.

Preparation for this PR included several prior PRs:

@rtyley rtyley changed the title Upgrade to Play v2.9 Upgrade to Play v2.9, Scala 2.13, Java 11 Jan 30, 2024
rtyley added a commit that referenced this pull request Jan 31, 2024
This removes the unmaintained bizzabo/diff library ("ai.x" %% "diff") from media-atom-maker,
as a prelude to the Scala 2.13 upgrade in PR #1140 (the diff library is only available for
Scala 2.11 & 2.12).

The library is only used for the media-atom-maker audit log, logging a diff when certain
fields on MediaAtom entities change. The diff text only goes to the ELK logs in the
free-format message & description fields.

Originally, there was some functionality in the media-atom-maker UI involving an audit
button, but this was removed with #759 in February 2018, and the ELK became the place this
information was sent to (the only place) with #767. See also #868 (comment) for a bit of
additional context.

All this means that the exact format of the diff logged to the ELK is not that important -
right now, the information could only really be used by a developer as an informative
diagnostic - there is no process that cares if format changes.

We looked at using alternative diffing libraries (diffx or difflicious) to produce the diff,
but getting them to produce the format we wanted (a one-line diff with only changed fields
taken from an accept-list, and no colouring) looked like it would take a bit of work - so,
given that there were only a few fields to look at, we wrote a custom diff function, and
extracted the diffing method out, so it could be easily unit-tested.
rtyley added a commit that referenced this pull request Jan 31, 2024
This removes the unmaintained bizzabo/diff library ("ai.x" %% "diff") from media-atom-maker,
as a prelude to the Scala 2.13 upgrade in PR #1140 (the diff library is only available for
Scala 2.11 & 2.12).

The library is only used for the media-atom-maker audit log, logging a diff when certain
fields on MediaAtom entities change. The diff text only goes to the ELK logs in the
free-format message & description fields.

Originally, there was some functionality in the media-atom-maker UI involving an audit
button, but this was removed with #759 in February 2018, and the ELK became the place this
information was sent to (the only place) with #767. See also #868 (comment) for a bit of
additional context.

All this means that the exact format of the diff logged to the ELK is not that important -
right now, the information could only really be used by a developer as an informative
diagnostic - there is no process that cares if format changes.

We looked at using alternative diffing libraries (diffx or difflicious) to produce the diff,
but getting them to produce the format we wanted (a one-line diff with only changed fields
taken from an accept-list, and no colouring) looked like it would take a bit of work - so,
given that there were only a few fields to look at, we wrote a custom diff function, and
extracted the diffing method out, so it could be easily unit-tested.
@rtyley rtyley force-pushed the upgrade-to-play-v2.9 branch from b798185 to 90afd35 Compare January 31, 2024 11:28
@rtyley rtyley changed the base branch from main to switch-diff-summary-library January 31, 2024 11:29
@rtyley rtyley force-pushed the upgrade-to-play-v2.9 branch 3 times, most recently from 9c43f2d to 3a13d5b Compare January 31, 2024 16:57
@rtyley rtyley force-pushed the switch-diff-summary-library branch from f60f0b7 to 0d0535c Compare January 31, 2024 16:57
Base automatically changed from switch-diff-summary-library to main February 1, 2024 10:57
rtyley added a commit that referenced this pull request Feb 1, 2024
This removes the unmaintained bizzabo/diff library ("ai.x" %% "diff") from media-atom-maker,
as a prelude to the Scala 2.13 upgrade in PR #1140 (the diff library is only available for
Scala 2.11 & 2.12).

The library is only used for the media-atom-maker audit log, logging a diff when certain
fields on MediaAtom entities change. The diff text only goes to the ELK logs in the
free-format message & description fields.

Originally, there was some functionality in the media-atom-maker UI involving an audit
button, but this was removed with #759 in February 2018, and the ELK became the place this
information was sent to (the only place) with #767. See also #868 (comment) for a bit of
additional context.

All this means that the exact format of the diff logged to the ELK is not that important -
right now, the information could only really be used by a developer as an informative
diagnostic - there is no process that cares if format changes.

We looked at using alternative diffing libraries (diffx or difflicious) to produce the diff,
but getting them to produce the format we wanted (a one-line diff with only changed fields
taken from an accept-list, and no colouring) looked like it would take a bit of work - so,
given that there were only a few fields to look at, we wrote a custom diff function, and
extracted the diffing method out, so it could be easily unit-tested.
rtyley added a commit that referenced this pull request Feb 1, 2024
This upgrades the Media Atom Maker to use the latest version of
the client for the Guardian's Editorial Permissions service - we
need the latest version of the client to support the upgrade to
Scala 2.13 in #1140

* Before: https://github.com/guardian/editorial-permissions-client/tree/v0.8 - supporting Scala 2.11 & 2.12
* After: https://github.com/guardian/permissions/tree/v2.15/client - supporting Scala 2.12 & 2.13

As you can see, the permissions client has moved repositories, to the
main `permissions` repo - this happened in July 2018 with PR
guardian/permissions#103. This PR is also
important because it removed use of `Future` from the permissions
client API - as Michael Barton explained, permission lookups should
be mostly instantaneous because they now come from an in-memory cache.

The removal of `Future` means that this commit, upgrading permissions in
Media Atom Maker, needs to remove several for-comprehensions/map-statements.
The diff on these can look quite big, but they look much smaller if whitespace
changes are ignored.

# Permission to change Privacy Status of a Media Atom

* #789
* #791
rtyley added a commit that referenced this pull request Feb 1, 2024
This upgrades the Media Atom Maker to use the latest version of
the client for the Guardian's Editorial Permissions service - we
need the latest version of the client to support the upgrade to
Scala 2.13 in #1140

* Before: https://github.com/guardian/editorial-permissions-client/tree/v0.8 - supporting Scala 2.11 & 2.12
* After: https://github.com/guardian/permissions/tree/v2.15/client - supporting Scala 2.12 & 2.13

As you can see, the permissions client has moved repositories, to the
main `permissions` repo - this happened in July 2018 with PR
guardian/permissions#103. This PR is also
important because it removed use of `Future` from the permissions
client API - as Michael Barton explained, permission lookups should
be mostly instantaneous because they now come from an in-memory cache.

The removal of `Future` means that this commit, upgrading permissions in
Media Atom Maker, needs to remove several for-comprehensions/map-statements.
The diff on these can look quite big, but they look smaller if whitespace
changes are ignored. I have taken the opportunity to do small refactors to
improve code clarity and remove repetition.

# Permission to change Privacy Status of a Media Atom

In particular, the code around changing Privacy Status of a Media Atom
_had_ to be changed because it involved removing `Future`, but I also
included refactoring to make the code clearer. When reviewing this,
you may want to look at the original PRs that introduced this logic:

* #789
* #791

A summary of the logic is:

"Once a video is public, it should remain public. Otherwise when someone
without permission edits a title/thumbnail, video becomes unlisted"
rtyley added a commit that referenced this pull request Feb 1, 2024
This upgrades the Media Atom Maker to use the latest version of
the client for the Guardian's Editorial Permissions service - we
need the latest version of the client to support the upgrade to
Scala 2.13 in #1140

* Before: https://github.com/guardian/editorial-permissions-client/tree/v0.8 - supporting Scala 2.11 & 2.12
* After: https://github.com/guardian/permissions/tree/v2.15/client - supporting Scala 2.12 & 2.13

As you can see, the permissions client has moved repositories, to the
main `permissions` repo - this happened in July 2018 with PR
guardian/permissions#103. This PR is also
important because it removed use of `Future` from the permissions
client API - as Michael Barton explained, permission lookups should
be mostly instantaneous because they now come from an in-memory cache.

The removal of `Future` means that this commit, upgrading permissions in
Media Atom Maker, needs to remove several for-comprehensions/map-statements.
The diff on these can look quite big, but they look smaller if whitespace
changes are ignored. I have taken the opportunity to do small refactors to
improve code clarity and remove repetition.

# Permission to change Privacy Status of a Media Atom

In particular, the code around changing Privacy Status of a Media Atom
_had_ to be changed because it involved removing `Future`, but I also
included refactoring to make the code clearer. When reviewing this,
you may want to look at the original PRs that introduced this logic:

* #789
* #791

A summary of the logic is:

* Once a video is public, it should remain public.
* When someone *without permission to make a video public* makes an edit
  (eg a minor metadata edit of title/thumbnail) on a published public
  video, the video should _not_ become unlisted.
rtyley added a commit that referenced this pull request Feb 1, 2024
This upgrades the Media Atom Maker to use the latest version of
the client for the Guardian's Editorial Permissions service - we
need the latest version of the client to support the upgrade to
Scala 2.13 in #1140

* Before: https://github.com/guardian/editorial-permissions-client/tree/v0.8 - supporting Scala 2.11 & 2.12
* After: https://github.com/guardian/permissions/tree/v2.15/client - supporting Scala 2.12 & 2.13

As you can see, the permissions client has moved repositories, to the
main `permissions` repo - this happened in July 2018 with PR
guardian/permissions#103. This PR is also
important because it removed use of `Future` from the permissions
client API - as Michael Barton explained, permission lookups should
be mostly instantaneous because they now come from an in-memory cache.

The removal of `Future` means that this commit, upgrading permissions in
Media Atom Maker, needs to remove several for-comprehensions/map-statements.
The diff on these can look quite big, but they look smaller if whitespace
changes are ignored. I have taken the opportunity to do small refactors to
improve code clarity and remove repetition.

# Permission to change Privacy Status of a Media Atom

In particular, the code around changing Privacy Status of a Media Atom
_had_ to be changed because it involved removing `Future`, but I also
included refactoring to make the code clearer. When reviewing this,
you may want to look at the original PRs that introduced this logic:

* #607 - introduced
  the concept of each of our YouTube channels having a different set of
  available PrivacyStatus (Private, Unlisted, Public) values.
* #694 - you can always
  upload as Public unless the channel is in the youtube.channels.unlisted
  config, in which case you need permission. This means we can give general
  users the ability to upload as Public on the culture channel and grant
  specific people access to make a public video on the main channel.
* #789 - public video
  should stay public when a metadata change is made by someone who
  does not have permission to *make* a video public on that channel.
* #791 - code shouldn't
  fail if the atom has not been published yet!
rtyley added a commit that referenced this pull request Feb 1, 2024
This upgrades the Media Atom Maker to use the latest version of
the client for the Guardian's Editorial Permissions service - we
need the latest version of the client to support the upgrade to
Scala 2.13 in #1140

* Before: https://github.com/guardian/editorial-permissions-client/tree/v0.8 - supporting Scala 2.11 & 2.12
* After: https://github.com/guardian/permissions/tree/v2.15/client - supporting Scala 2.12 & 2.13

As you can see, the permissions client has moved repositories, to the
main `permissions` repo - this happened in July 2018 with PR
guardian/permissions#103. This PR is also
important because it removed use of `Future` from the permissions
client API - as Michael Barton explained, permission lookups should
be mostly instantaneous because they now come from an in-memory cache.

The removal of `Future` means that this commit, upgrading permissions in
Media Atom Maker, needs to remove several for-comprehensions/map-statements.
The diff on these can look quite big, but they look smaller if whitespace
changes are ignored. I have taken the opportunity to do small refactors to
improve code clarity and remove repetition.

# Permission to modify Privacy Status of a published Media Atom

In particular, the code around modifying Privacy Status of a Media Atom
_had_ to be changed because it involved removing `Future`, but I also
included refactoring to make the code clearer. When reviewing this,
you may want to look at the original PRs that introduced this logic:

* #607 - introduced
  the concept of each of our YouTube channels having a different set of
  available PrivacyStatus (Private, Unlisted, Public) values.
* #694 - you can always
  upload as Public unless the channel is in the youtube.channels.unlisted
  config, in which case you need permission. This means we can give general
  users the ability to upload as Public on the culture channel and grant
  specific people access to make a public video on the main channel.
* #789 - public video
  should *stay* public when a metadata change is made by someone who
  does not have permission to *make* a video public on that channel.
* #791 - code shouldn't
  fail if the atom has not been published yet!
@rtyley rtyley force-pushed the upgrade-to-play-v2.9 branch from 3a13d5b to 0c948bf Compare February 1, 2024 13:15
@rtyley rtyley changed the base branch from main to upgrade-permissions-library February 1, 2024 13:16
@rtyley rtyley force-pushed the upgrade-permissions-library branch from 8316075 to 9abd55e Compare February 1, 2024 14:02
@rtyley rtyley force-pushed the upgrade-to-play-v2.9 branch from 082b9a2 to 9167da7 Compare February 1, 2024 14:02
@rtyley rtyley force-pushed the upgrade-to-play-v2.9 branch from 9167da7 to d328c60 Compare February 1, 2024 14:06
@rtyley rtyley force-pushed the upgrade-to-play-v2.9 branch from 7e5bb61 to c480c2c Compare February 2, 2024 17:22
@rtyley rtyley changed the base branch from upgrade-permissions-library to pending-pre-scala-2.13-prs February 2, 2024 17:24
@rtyley rtyley force-pushed the upgrade-to-play-v2.9 branch 2 times, most recently from 6e482ea to 8e2d291 Compare February 2, 2024 17:43
rtyley added a commit that referenced this pull request Feb 8, 2024
This is a pre-requisite for the work upgrading to Scala 2.13 in
#1140.

Note that the panda-hmac-play artifacts have been moved out
of the archived https://github.com/guardian/panda-hmac repo,
with the last version there being v2.2.0 released in June 2023.
They are now in the main https://github.com/guardian/pan-domain-authentication
repo, and releases are now done with the other panda artifacts -
so the version numbers of all the panda artifacts, including
panda-hmac, are now aligned.
@rtyley rtyley force-pushed the upgrade-to-play-v2.9 branch 2 times, most recently from 62b654a to fa27e4a Compare February 8, 2024 14:16
rtyley added a commit that referenced this pull request Feb 8, 2024
This is in preparation for the Play 2.9 upgrade in
#1140 - the current
Panda artifacts used actually _do_ have Scala 2.13 versions at the
current artifact version number:

- https://repo1.maven.org/maven2/com/gu/pan-domain-auth-play_2-8_2.13/1.2.0/
- https://repo1.maven.org/maven2/com/gu/panda-hmac-play_2-8_2.13/2.1.0/

...but they _don't_ have Play 2.9 versions, not at this artifact number -
so for the later upgrade to Play 2.9, we will need to update the Panda
artifact version number, and it makes sense to separate this change out
to make the later change smaller.

Note that the panda-hmac-play artifacts have been moved out
of the archived https://github.com/guardian/panda-hmac repo,
with the last version there being v2.2.0 released in June 2023.
They are now in the main https://github.com/guardian/pan-domain-authentication
repo, and releases are now done with the other panda artifacts -
so the version numbers of all the panda artifacts, including
panda-hmac, are now aligned.
@rtyley rtyley changed the base branch from pending-pre-scala-2.13-prs to upgrade-panda February 8, 2024 16:25
@rtyley rtyley added the Play 2.9 Upgrade to Playframework v2.9 label Feb 8, 2024
@rtyley rtyley force-pushed the upgrade-to-play-v2.9 branch 2 times, most recently from de172dc to f170fff Compare February 8, 2024 16:35
@rtyley rtyley changed the title Upgrade to Play v2.9, Scala 2.13, Java 11 Upgrade to Play v2.9, Java 11 Feb 8, 2024
@rtyley rtyley mentioned this pull request Feb 8, 2024
@rtyley rtyley changed the base branch from upgrade-panda to upgrade-to-scala-v2.13 February 8, 2024 17:40
@rtyley rtyley removed the Scala 2.13 label Feb 8, 2024
@rtyley rtyley force-pushed the upgrade-to-scala-v2.13 branch 2 times, most recently from 3eed1ee to eea3072 Compare February 9, 2024 08:42
@rtyley rtyley force-pushed the upgrade-to-play-v2.9 branch from f170fff to 97fe374 Compare February 9, 2024 08:43
Base automatically changed from upgrade-to-scala-v2.13 to main February 9, 2024 09:56
Comment thread build.sbt Outdated
val awsV2Version = "2.21.17"
val pandaVersion = "3.0.1"
val atomMakerVersion = "1.3.4"
val atomMakerVersion = "2.0.0-PREVIEW.fix-tests-under-java-17.2024-01-18T1039.b4d55b3d"
Copy link
Copy Markdown
Member Author

@rtyley rtyley Feb 9, 2024

Choose a reason for hiding this comment

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

This is a preview release of guardian/atom-maker#94 (specifically compiling that library against Play 2.9) which should be merged before this PR, so that we can just use a proper full release version:

Suggested change
val atomMakerVersion = "2.0.0-PREVIEW.fix-tests-under-java-17.2024-01-18T1039.b4d55b3d"
val atomMakerVersion = "2.0.0"

@rtyley rtyley force-pushed the upgrade-to-play-v2.9 branch 3 times, most recently from 6050b9f to 00f8898 Compare February 9, 2024 15:30
@rtyley rtyley changed the base branch from main to remove-superfluous-sbt-coursier-plugin February 9, 2024 15:31
@rtyley rtyley marked this pull request as ready for review February 9, 2024 15:35
@rtyley rtyley mentioned this pull request Feb 9, 2024
Comment thread project/plugins.sbt
Comment on lines -22 to -31

/*
scala-xml has been updated to 2.x in sbt, but not in other sbt plugins like sbt-native-packager
See: https://github.com/scala/bug/issues/12632
This is effectively overrides the safeguards (early-semver) put in place by the library authors ensuring binary compatibility.
We consider this a safe operation because when set under `projects/` (ie *not* in `build.sbt` itself) it only affects the
compilation of build.sbt, not of the application build itself.
Once the build has succeeded, there is no further risk (ie of a runtime exception due to clashing versions of `scala-xml`).
*/
libraryDependencySchemes += "org.scala-lang.modules" %% "scala-xml" % VersionScheme.Always
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is no longer needed, as all dependencies, including Play 2.9, are using compatible versions of scala-xml.

Copy link
Copy Markdown
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.

Look good, tested in CODE and working nicely.

@prout-bot
Copy link
Copy Markdown

Seen on PROD (created by @rtyley and merged by @rhystmills 6 minutes and 28 seconds ago) Please check your changes!

@rtyley
Copy link
Copy Markdown
Member Author

rtyley commented Feb 14, 2024

This caused problems for the media-atom-maker-expirer & media-atom-maker-scheduler lambdas, as they needed to be updated to use the Java 11 runtime, but in #1149 we'd not realised we needed to update the cloudformation in editorial-tools-platform as well, as ultimately done in https://github.com/guardian/editorial-tools-platform/pull/760.

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

Labels

Play 2.9 Upgrade to Playframework v2.9 Seen-on-PROD

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants