#77: scala2.13 support #78

lsulak · 2023-09-22T15:12:44Z

Closes #77

benedeki · 2023-10-13T11:30:05Z

build.sbt

@@ -68,6 +68,15 @@ lazy val server = (projectMatrix in file("server"))
      libraryDependencies ++= Dependencies.serverDependencies,
      Compile / packageBin / publishArtifact := false,
      (Compile / compile) := ((Compile / compile) dependsOn printSparkScalaVersion).value,
+      Compile / unmanagedSourceDirectories += {


Server is not a lib to include but a running app. I would simply build it with Scala 2.13 atm

Actually no. Keep it only 2.12. fa-db doesn't support 2.13 yet. :-(

You are right - I've changed a thing or two. Now only the agent (and model) is being compiled against all 3 versions, the server is compiled only for 2.12.

The sbt log looks something like this:

[info] compiling 7 Scala sources to /Users/ab0222l/Documents/git/atum-service/model/target/jvm-2.12/classes ... [info] compiling 7 Scala sources to /Users/ab0222l/Documents/git/atum-service/model/target/jvm-2.13/classes ... [info] compiling 7 Scala sources to /Users/ab0222l/Documents/git/atum-service/model/target/jvm-2.11/classes ... [info] compiling 25 Scala sources to /Users/ab0222l/Documents/git/atum-service/server/target/jvm-2.12/classes ... [info] compiling 5 Scala sources to /Users/ab0222l/Documents/git/atum-service/agent/target/spark3-jvm-2.12/classes ... [info] compiling 5 Scala sources to /Users/ab0222l/Documents/git/atum-service/agent/target/jvm-2.12/classes ... [info] compiling 5 Scala sources to /Users/ab0222l/Documents/git/atum-service/agent/target/jvm-2.13/classes ... [info] compiling 5 Scala sources to /Users/ab0222l/Documents/git/atum-service/agent/target/spark2-jvm-2.12/classes ... [info] compiling 5 Scala sources to /Users/ab0222l/Documents/git/atum-service/agent/target/spark3-jvm-2.13/classes ... [info] compiling 5 Scala sources to /Users/ab0222l/Documents/git/atum-service/agent/target/jvm-2.11/classes ... [info] compiling 5 Scala sources to /Users/ab0222l/Documents/git/atum-service/agent/target/spark2-jvm-2.11/classes ...

…t (and model) in 2.11, 2.12, and 2.13

Zejnilovic · 2023-10-18T22:36:29Z

https://github.com/AbsaOSS/atum-service/blob/cdf18c2c5073b4b339178aa0ccfa8cb20844a93d/.github/workflows/jacoco_check.yml#L36C27-L36C27

Please update this to from 2.12.12 to 2.12

github-actions · 2023-10-19T05:24:19Z

JaCoCo agent module code coverage report - spark:2 - scala 2.12.18

There is no coverage information present for the Files changed

Total Project Coverage	83.32%	🍏

github-actions · 2023-10-19T05:24:20Z

JaCoCo server module code coverage report - scala 2.12.18

File	Coverage [7.69%]	❌
InMemoryApiModelDao.scala	7.69%	❌

Total Project Coverage	17.34%	❌

miroslavpojer · 2023-10-19T05:25:17Z

https://github.com/AbsaOSS/atum-service/blob/cdf18c2c5073b4b339178aa0ccfa8cb20844a93d/.github/workflows/jacoco_check.yml#L36C27-L36C27

Please update this to from 2.12.12 to 2.12

I have updated it to 2.12.18. That fixed missing JaCoCo comments.

benedeki

code reviewed
pulled
built
run

benedeki · 2023-10-24T22:40:55Z

project/Dependencies.scala

+    val scala212 = "2.12.18"
+    val scala213 = "2.13.11"
+
+    val serverScalaVersion: String = scala212


Why is this not a Seq as well as aqentSupportedVersions?

Because supporting more versions in the server is not needed imho, i.e. there will be probably just 1 instance of server, managed by 'us', deployed into Docker & ECR repo & EKS, and it will be able to communicate with the agent that might be in any version - it's just a REST API communication between them after all

but on the other hand, agent will be used inside of our applications' scala code, which are usually in range of Scala 2.11 - 2.13 so that must be a Seq

Also, is there a reason for sever not being 2.13?

Yes, usually everything in my code has a reason although sometimes not a good one :D anyway, in this case, server has a dependency on fa-db and that is not yet on 2.13 unfortunately

Because supporting more versions in the server is not needed imho, i.e. there will be probably just 1 instance of server, managed by 'us', deployed into Docker & ECR repo & EKS, and it will be able to communicate with the agent that might be in any version - it's just a REST API communication between them after all

@lsulak I get it, but then you have 2 different implementations. I would have it both as Seq to have it the same across. First look you see one as Seq and one not.

Why to have something as a different, and improper, data type, just for the sake of consistency? To the best of my knowledge, server will always be in 1 version, thus any kind of iterable doesn't make sense

I like the way how it is.
But the DB tests will also suffice to exist under one Scala version. On the other hand the Model module is multi-versioned. So maybe a rename of the variable to more neutral ones?

.github/workflows/jacoco_check.yml

build.sbt

Zejnilovic · 2023-10-25T07:53:06Z

build.sbt

@@ -58,6 +57,7 @@ lazy val root = (projectMatrix in file("."))
    name := "atum-service-root",
    javacOptions ++= Seq("-source", "1.8", "-target", "1.8", "-Xlint"),
    publish / skip := true,
+    crossScalaVersions := Nil,
    mergeStrategy


Are we using sbt assembly for something?

I guess that there will be Jar containing server and its configuration, and the model, nicely packaged together with it

I wanna make a bold comment that you do not need mergeStrategy at all. Or at least I have not seen it.

build.sbt

…o feature/77-scala212-support

miroslavpojer · 2023-10-31T09:48:01Z

https://github.com/AbsaOSS/atum-service/blob/cdf18c2c5073b4b339178aa0ccfa8cb20844a93d/.github/workflows/jacoco_check.yml#L36C27-L36C27

Please update this to from 2.12.12 to 2.12

I have tested this "requirement" on another GHA workflow and it does not work. The not shorted scala version string is required in titles (can be changed, here) and in sbt ++{scala.version} call, where it is required.

miroslavpojer

pulled
code review
run test
run JaCoCo

Optional: This is nice PR to do some atomic increment increase of code coverage.

…-support # Conflicts: # project/Dependencies.scala

lsulak · 2023-11-09T13:22:52Z

Oopsie. We recently added (outside of this PR) logging via SLF4S, but that's not yet compatible with Scala 2.13 :( mattroberts297/slf4s#20

benedeki · 2023-11-10T11:13:07Z

project/Dependencies.scala

+    val scala212 = "2.12.18"
+    val scala213 = "2.13.11"
+
+    val serverScalaVersion: String = scala212


I like the way how it is.
But the DB tests will also suffice to exist under one Scala version. On the other hand the Model module is multi-versioned. So maybe a rename of the variable to more neutral ones?

benedeki · 2023-11-10T11:13:54Z

code reviewed
pulled
built
run

#77: Adding support for Scala 2.13 and improving the overall build

c1839b5

lsulak requested review from benedeki, TebaleloS, Zejnilovic and dk1844 as code owners September 22, 2023 15:12

lsulak requested a review from cerveada September 22, 2023 15:12

lsulak changed the base branch from master to feature/37-dao-between-agent-server-adjustments September 22, 2023 15:13

lsulak changed the title ~~#77: scala212 support~~ #77: scala2.13 support Sep 22, 2023

Base automatically changed from feature/37-dao-between-agent-server-adjustments to master October 6, 2023 15:55

cerveada previously approved these changes Oct 13, 2023

View reviewed changes

benedeki reviewed Oct 13, 2023

View reviewed changes

#77: post-review changes - making Server only in Scala 2.12, but agen…

cdf18c2

…t (and model) in 2.11, 2.12, and 2.13

lsulak dismissed cerveada’s stale review via cdf18c2 October 17, 2023 12:37

* Fix - trying to fix scala version.

8888899

benedeki previously approved these changes Oct 24, 2023

View reviewed changes

Zejnilovic reviewed Oct 25, 2023

View reviewed changes

.github/workflows/jacoco_check.yml Show resolved Hide resolved

Zejnilovic reviewed Oct 25, 2023

View reviewed changes

benedeki mentioned this pull request Oct 25, 2023

#83: Serde for DTO objects #93

Merged

Merge branch 'master' into feature/77-scala212-support

89f95ba

lsulak requested a review from salamonpavel as a code owner October 26, 2023 16:19

lsulak added 2 commits October 26, 2023 18:40

#77: post-review change

566eedb

Merge remote-tracking branch 'origin/feature/77-scala212-support' int…

6926783

…o feature/77-scala212-support

lsulak dismissed benedeki’s stale review via 6926783 October 26, 2023 16:40

Merge branch 'master' into feature/77-scala212-support

0a2275d

miroslavpojer reviewed Oct 31, 2023

View reviewed changes

lsulak and others added 4 commits October 31, 2023 17:25

Merge branch 'master' into feature/77-scala212-support

8a03940

Merge branch 'master' into feature/77-scala212-support

10cce71

* increased spring patch version

771e133

Merge remote-tracking branch 'origin/master' into feature/77-scala212…

cdc74ae

…-support # Conflicts: # project/Dependencies.scala

fixing build

a1a67f3

benedeki previously approved these changes Nov 10, 2023

View reviewed changes

Zejnilovic previously approved these changes Nov 10, 2023

View reviewed changes

#77: post-review refactorings

04cc86a

lsulak dismissed stale reviews from Zejnilovic and benedeki via 04cc86a November 10, 2023 13:47

benedeki approved these changes Nov 10, 2023

View reviewed changes

lsulak merged commit d8ead20 into master Nov 10, 2023
7 of 8 checks passed

lsulak deleted the feature/77-scala212-support branch November 10, 2023 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#77: scala2.13 support #78

#77: scala2.13 support #78

lsulak commented Sep 22, 2023

benedeki Oct 13, 2023

benedeki Oct 13, 2023

lsulak Oct 17, 2023

Zejnilovic commented Oct 18, 2023

github-actions bot commented Oct 19, 2023 •

edited

Loading

github-actions bot commented Oct 19, 2023 •

edited

Loading

miroslavpojer commented Oct 19, 2023

benedeki left a comment

benedeki Oct 24, 2023

Zejnilovic Oct 25, 2023

lsulak Oct 25, 2023

lsulak Oct 25, 2023

Zejnilovic Oct 25, 2023

lsulak Oct 25, 2023

Zejnilovic Oct 27, 2023

lsulak Nov 9, 2023

benedeki Nov 10, 2023

Zejnilovic Oct 25, 2023

lsulak Oct 26, 2023

Zejnilovic Oct 27, 2023

miroslavpojer commented Oct 31, 2023

miroslavpojer left a comment •

edited

Loading

lsulak commented Nov 9, 2023

benedeki Nov 10, 2023

benedeki commented Nov 10, 2023

#77: scala2.13 support #78

#77: scala2.13 support #78

Conversation

lsulak commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zejnilovic commented Oct 18, 2023

github-actions bot commented Oct 19, 2023 • edited Loading

JaCoCo agent module code coverage report - spark:2 - scala 2.12.18

github-actions bot commented Oct 19, 2023 • edited Loading

JaCoCo server module code coverage report - scala 2.12.18

miroslavpojer commented Oct 19, 2023

benedeki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miroslavpojer commented Oct 31, 2023

miroslavpojer left a comment • edited Loading

Choose a reason for hiding this comment

lsulak commented Nov 9, 2023

Choose a reason for hiding this comment

benedeki commented Nov 10, 2023

github-actions bot commented Oct 19, 2023 •

edited

Loading

github-actions bot commented Oct 19, 2023 •

edited

Loading

miroslavpojer left a comment •

edited

Loading