Upgrade to java11 #102

lindseydew · 2024-02-29T17:43:30Z

What does this change?

This upgrades the lambdas that serve the Save For Later functionality from Java8 to Java11

How to test

Tested Save for Later on CODE:

Can get articles
Can post articles

Tested User Deletion CODE:

Delete a user account and see that this is deleted from Save For Later

Note I wasn't able to test the full interaction on CODE as I have been unable to find the exact string the app is expecting to parse. I think the json parsing logic needs a bit of an update too, so I'll make a note to do that and this should make it possible to test an example event.

For now, we have alarms set up on PROD if the delete interaction is unable to complete.

How can we measure success?

Have we considered potential risks?

Images

Accessibility

mchv · 2024-03-01T11:34:18Z

project/dependencies.scala

+  val jacksonDataFormat =
+    "com.fasterxml.jackson.dataformat" % "jackson-dataformat-cbor" % jacksonVersion
+  val jacksonJdk8DataType =
+    "com.fasterxml.jackson.datatype" % "jackson-datatype-jdk8" % jacksonVersion


I think this may be removed

mchv · 2024-03-01T12:06:45Z

@lindseydew I noticed a few additional changes linked to this upgrade which I have directly added as additional commits..but the build is now breaking 😢! I think this is because sbt-assembly had changed its API, and it should not be too hard to fix existing file using documentation (but I have not done it!).

Anyway once this is fixed, the upgrade to java 21 should be straightforward as it supported by latest scala 2.12.x and sbt 1.9.9, and so require only a few changes.

lindseydew · 2024-03-06T12:15:02Z

@lindseydew I noticed a few additional changes linked to this upgrade which I have directly added as additional commits..but the build is now breaking 😢! I think this is because sbt-assembly had changed its API, and it should not be too hard to fix existing file using documentation (but I have not done it!).

Anyway once this is fixed, the upgrade to java 21 should be straightforward as it supported by latest scala 2.12.x and sbt 1.9.9, and so require only a few changes.

Great thank you, I'll take a look at the build

waisingyiu · 2024-03-13T11:57:04Z

mobile-save-for-later-user-deletion/template.yml

@@ -0,0 +1,84 @@
+# This AWS SAM template has been generated from your function's configuration. If


Is it the cloudformation for mobile-save-for-later? Why do we need this one when there is "conf/cfn.yaml"?

waisingyiu · 2024-03-13T11:58:33Z

mobile-save-for-later/template.yml

@@ -0,0 +1,97 @@
+# This AWS SAM template has been generated from your function's configuration. If


I think CDK is used to define the cloudformation stack for this service? Is it used for another purpose?

Yeah, we are using the CDK for the cfn stack definition. This is used to be able to run the lambda functions locally using aws sam. Instructions here: https://github.com/guardian/mobile-save-for-later/pull/102/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R86

waisingyiu · 2024-03-13T12:07:09Z

build.sbt

    case "META-INF/MANIFEST.MF" => MergeStrategy.discard
-    case "META-INF/org/apache/logging/log4j/core/config/plugins/Log4j2Plugins.dat" =>
-      new MergeFilesStrategy
+    case PathList(ps @ _*) if ps.last equalsIgnoreCase "Log4j2Plugins.dat" =>


You may have explained this in our snyk session, but I am not sure why we change the implementation of merge strategy here? The previous one does not work?

yeah, I was getting an error with instantiating the MergeFilesStrategy class.

waisingyiu

Thank you for the great work, I am happy to approve.

As an aside, I suppose that I need a docker desktop license if I want to run this service locally? (I see your name on the docker desktop license spreadsheet)

rtyley

I've only quickly skimmed through the diff of this PR, as I'm a bit short of time today!

I think the diff might be a bit bigger than it needs to be, I think because scalafmt formatting changes are being included - that makes it a bit harder to concentrate on the significant code changes? If scalafmt is desired, it might be worth doing a separate scalafmt PR and then rebasing this one on top of that.

@lindseydew, you suggested that maybe should merge this PR, and then merge #105, which is understandable because #105 talks about Java 21, and this PR is for Java 11!

However, #105 only allows mobile-save-for-later to build under Java 21, doesn't require it, and doesn't even attempt to update mobile-save-for-later up from Java 8 - consequently, it's a much smaller PR, easier to review, and therefore, I would say, much lower risk!

So personally, I would prefer to merge #105 immediately, and then look at rebasing this PR on top of main - maybe even separating out a scalafmt run into it's own PR first. I'd be really happy to pair tomorrow on those stages of the operation if that would help?

build.sbt

lindseydew · 2024-03-19T15:15:37Z

I've only quickly skimmed through the diff of this PR, as I'm a bit short of time today!

I think the diff might be a bit bigger than it needs to be, I think because scalafmt formatting changes are being included - that makes it a bit harder to concentrate on the significant code changes? If scalafmt is desired, it might be worth doing a separate scalafmt PR and then rebasing this one on top of that.

@lindseydew, you suggested that maybe should merge this PR, and then merge #105, which is understandable because #105 talks about Java 21, and this PR is for Java 11!

However, #105 only allows mobile-save-for-later to build under Java 21, doesn't require it, and doesn't even attempt to update mobile-save-for-later up from Java 8 - consequently, it's a much smaller PR, easier to review, and therefore, I would say, much lower risk!

So personally, I would prefer to merge #105 immediately, and then look at rebasing this PR on top of main - maybe even separating out a scalafmt run into it's own PR first. I'd be really happy to pair tomorrow on those stages of the operation if that would help?

Ok sounds good. Yes I'm happy for you to merge your branch first. Agree with separating the PR. A pairing session would be great. When would you be free?

A few changes: - update to latest checkout action - update to latest configure-aws-credentials action - update to latest setup-jave - use corretto recommended distribution

Co-authored-by: Roberto Tyley <[email protected]>

lindseydew · 2024-03-25T11:21:28Z

Closing because this has been superseded by subsequent PRs: #106 and #107

lindseydew requested a review from waisingyiu February 29, 2024 17:43

mchv reviewed Mar 1, 2024

View reviewed changes

waisingyiu reviewed Mar 13, 2024

View reviewed changes

waisingyiu approved these changes Mar 13, 2024

View reviewed changes

rtyley reviewed Mar 19, 2024

View reviewed changes

build.sbt Show resolved Hide resolved

lindseydew and others added 20 commits March 20, 2024 09:41

Update libs for java11

11504a8

Make compile with explicit dynamo format override

5083261

Move the implicit val to the companion object

984b165

Implement the dynamo formatter

6981976

Update the cfn to java11

57a6c49

Add instructions for running locally

8b13389

Update local java version

ccdc3df

Remove 1.8 as target runtime

499c1ec

Update to latest SBT assembly plugin

af94449

Update to latest SBT version

e77da1b

Update to Java 11

6696cd6

A few changes: - update to latest checkout action - update to latest configure-aws-credentials action - update to latest setup-jave - use corretto recommended distribution

Update sbt assembly

45b90fe

Update readme

ad862c7

Remove the jackson jdk8 data type

86e5b4a

Remove jacksonJdk8DataType ref from build.sbt

dca2808

Remove where lib is used

be40fd2

Add notes for user deletion testing on CODE

ce33ffe

Use default creds provider

14926c6

create the creds

0db7d3f

Update user delete version + bump memory

3967a1b

lindseydew added 5 commits March 20, 2024 09:46

Update dynamo client creation

87d4ce3

Add in custom strategy for plugins

630d2c3

Log out errors and use update for new record

f7838c2

Use update instead of write

b044803

Run tests in CI

f115e24

lindseydew force-pushed the ld/upgrade-to-java11 branch from a340321 to f115e24 Compare March 20, 2024 09:46

Update build.sbt

0c356bc

Co-authored-by: Roberto Tyley <[email protected]>

lindseydew closed this Mar 25, 2024

lindseydew mentioned this pull request Mar 28, 2024

Upgrade to Java 11 #107

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to java11 #102

Upgrade to java11 #102

lindseydew commented Feb 29, 2024 •

edited

Loading

mchv Mar 1, 2024

mchv commented Mar 1, 2024

lindseydew commented Mar 6, 2024

waisingyiu Mar 13, 2024

waisingyiu Mar 13, 2024

lindseydew Mar 13, 2024

waisingyiu Mar 13, 2024

lindseydew Mar 13, 2024

waisingyiu left a comment

rtyley left a comment

lindseydew commented Mar 19, 2024

lindseydew commented Mar 25, 2024

		@@ -0,0 +1,84 @@
		# This AWS SAM template has been generated from your function's configuration. If

		@@ -0,0 +1,97 @@
		# This AWS SAM template has been generated from your function's configuration. If

Upgrade to java11 #102

Upgrade to java11 #102

Conversation

lindseydew commented Feb 29, 2024 • edited Loading

What does this change?

How to test

How can we measure success?

Have we considered potential risks?

Images

Accessibility

mchv Mar 1, 2024

Choose a reason for hiding this comment

mchv commented Mar 1, 2024

lindseydew commented Mar 6, 2024

waisingyiu Mar 13, 2024

Choose a reason for hiding this comment

waisingyiu Mar 13, 2024

Choose a reason for hiding this comment

lindseydew Mar 13, 2024

Choose a reason for hiding this comment

waisingyiu Mar 13, 2024

Choose a reason for hiding this comment

lindseydew Mar 13, 2024

Choose a reason for hiding this comment

waisingyiu left a comment

Choose a reason for hiding this comment

rtyley left a comment

Choose a reason for hiding this comment

lindseydew commented Mar 19, 2024

lindseydew commented Mar 25, 2024

lindseydew commented Feb 29, 2024 •

edited

Loading