Upgrade to Play v2.9#1140
Conversation
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.
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.
b798185 to
90afd35
Compare
9c43f2d to
3a13d5b
Compare
f60f0b7 to
0d0535c
Compare
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.
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
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"
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.
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!
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!
3a13d5b to
0c948bf
Compare
8316075 to
9abd55e
Compare
082b9a2 to
9167da7
Compare
9167da7 to
d328c60
Compare
7e5bb61 to
c480c2c
Compare
6e482ea to
8e2d291
Compare
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.
62b654a to
fa27e4a
Compare
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.
de172dc to
f170fff
Compare
3eed1ee to
eea3072
Compare
f170fff to
97fe374
Compare
| 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:
| val atomMakerVersion = "2.0.0-PREVIEW.fix-tests-under-java-17.2024-01-18T1039.b4d55b3d" | |
| val atomMakerVersion = "2.0.0" |
6050b9f to
00f8898
Compare
|
|
||
| /* | ||
| 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.
rhystmills
left a comment
There was a problem hiding this comment.
Look good, tested in CODE and working nicely.
|
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 |
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: