Upgrade to Play v2.9 #1140
Merged
Upgrade to Play v2.9 #1140
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rtyley
changed the title
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
force-pushed
the
upgrade-to-play-v2.9
branch
from
b798185
to
90afd35
Compare
rtyley
force-pushed
the
upgrade-to-play-v2.9
branch
3 times, most recently
from
9c43f2d
to
3a13d5b
Compare
rtyley
force-pushed
the
switch-diff-summary-library
branch
from
f60f0b7
to
0d0535c
Compare
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
force-pushed
the
upgrade-to-play-v2.9
branch
from
3a13d5b
to
0c948bf
Compare
rtyley
force-pushed
the
upgrade-permissions-library
branch
from
8316075
to
9abd55e
Compare
rtyley
force-pushed
the
upgrade-to-play-v2.9
branch
from
082b9a2
to
9167da7
Compare
rtyley
force-pushed
the
upgrade-to-play-v2.9
branch
from
9167da7
to
d328c60
Compare
rtyley
force-pushed
the
upgrade-to-play-v2.9
branch
from
7e5bb61
to
c480c2c
Compare
rtyley
changed the base branch from
upgrade-permissions-library
to
pending-pre-scala-2.13-prs
rtyley
force-pushed
the
upgrade-to-play-v2.9
branch
2 times, most recently
from
6e482ea
to
8e2d291
Compare
rtyley
commented
Feb 9, 2024
build.sbt
Outdated
|
|
||
| val scroogeVersion = "4.12.0" | ||
| val awsVersion = "1.11.678" | ||
| 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" |
There was a problem hiding this comment.
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
force-pushed
the
upgrade-to-play-v2.9
branch
3 times, most recently
from
6050b9f
to
00f8898
Compare
rtyley
changed the base branch from
main
to
remove-superfluous-sbt-coursier-plugin
rtyley
added a commit
that referenced
this pull request
Feb 9, 2024
This is a pre-requisite for the upcoming Play 2.9 upgrade in #1140 - Play 2.9 requires Java 11, 17, or 21 (and actually recommends Java 17). https://www.playframework.com/documentation/2.9.x/Requirements#Play-Requirements Upgrading from Java 8 to Java 11 has also been a DevX recommendation since 2022: https://docs.google.com/document/d/1ZR-YnaXCT5_gLVmTCeGs0mWd3KPaAozPjQK8uUzHZ9w/edit?usp=sharing See also: * https://github.com/guardian/atom-workshop/pull/349/files#r1457319862 * https://github.com/guardian/atom-workshop/pull/349/files#r1463164617
Merged
rtyley
force-pushed
the
upgrade-to-play-v2.9
branch
from
00f8898
to
8330a34
Compare
rtyley
changed the base branch from
remove-superfluous-sbt-coursier-plugin
to
upgrade-to-java-11
rtyley
force-pushed
the
upgrade-to-play-v2.9
branch
from
8330a34
to
624d9d0
Compare
rtyley
commented
Feb 9, 2024
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 |
There was a problem hiding this comment.
This is no longer needed, as all dependencies, including Play 2.9, are using compatible versions of scala-xml.
rtyley
added a commit
that referenced
this pull request
Feb 14, 2024
This is a pre-requisite for the upcoming Play 2.9 upgrade in #1140 - Play 2.9 requires Java 11, 17, or 21 (and actually recommends Java 17). https://www.playframework.com/documentation/2.9.x/Requirements#Play-Requirements Upgrading from Java 8 to Java 11 has also been a DevX recommendation since 2022: https://docs.google.com/document/d/1ZR-YnaXCT5_gLVmTCeGs0mWd3KPaAozPjQK8uUzHZ9w/edit?usp=sharing See also: * https://github.com/guardian/atom-workshop/pull/349/files#r1457319862 * https://github.com/guardian/atom-workshop/pull/349/files#r1463164617
rtyley
force-pushed
the
upgrade-to-play-v2.9
branch
from
624d9d0
to
6ea9469
Compare
rtyley
force-pushed
the
upgrade-to-play-v2.9
branch
from
6ea9469
to
aa21d88
Compare
rhystmills
approved these changes
Feb 14, 2024
|
Seen on PROD (created by @rtyley and merged by @rhystmills 6 minutes and 28 seconds ago) Please check your changes! |
|
This caused problems for the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: