update to jdk 11 [AS-594] #1372

zarsky-broad · 2021-03-01T20:42:03Z

Paired with https://github.com/broadinstitute/firecloud-develop/pull/2454...

zarsky-broad · 2021-03-08T22:52:00Z

Successful automation run (with paired firecloud-develop PR): https://fc-jenkins.dsp-techops.broadinstitute.org/job/swatomation-pipeline/46321/

zarsky-broad · 2021-03-09T17:09:15Z

project/Merging.scala

@@ -9,6 +9,7 @@ object Merging {
    case PathList("com", "typesafe", _ @ _*) => MergeStrategy.last
    case PathList("com", "google", "auto", "value", _ @ _*) => MergeStrategy.last
    case PathList("io", "sundr", _ @ _*) => MergeStrategy.last
+    case PathList("javax", "activation", _ @ _*) => MergeStrategy.first


This comes in through clients built with swagger codegen. I'm not positive that this is the most correct merge strategy, but I don't think it matters hugely.

Interesting. These generated client libraries are including the classes, not referring to them as a dependency? I'm surprised OpenAPI doesn't have support for transitive dependencies. Is there a link to an issue that can be included as a comment?

There's a series of issues referenced here that lead to Jackson and then to swagger-core, tbh I don't understand this all well enough to know what would be a useful comment.

There's a resolution to that last one, which will require updates to the version of swagger used to generate the clients, but I'm not exactly sure I understand what will need to happen.

I much prefer using exclude rules instead of the blunt-intstrument merge strategy. Do you know where the conflicts are, and could you add excludes for those instead?

fwiw, we recently also had assembly issues in orch on javax.activation, though that was via javax.mail: broadinstitute/firecloud-orchestration#838

I didn't experiment with that, I think I'd need some help to try it.

+1 for using exclusion rules

zarsky-broad · 2021-03-09T17:10:32Z

project/Testing.scala

@@ -22,6 +23,9 @@ object Testing {

  val commonTestSettings: Seq[Setting[_]] = List(

+    testOptions in Test += Tests.Setup(() =>
+      sys.props += "mockserver.logLevel" -> "WARN"


The default log verbosity for mockserver logs every mock defined, for every test that defines it. Since this goes to the console for jenkins jobs, it makes the logs huge, to the point that the browser has trouble searching through them.

zarsky-broad · 2021-03-09T17:12:33Z

project/Testing.scala

@@ -55,7 +59,7 @@ object Testing {
    (testOnly in Test) := ((testOnly in Test) dependsOn validMySqlHost).evaluated,

    parallelExecution in Test := false
-  ) ++ MinnieKenny.testSettings
+  ) ++ (if (sys.props.getOrElse("secrets.skip", false) != "true") MinnieKenny.testSettings else List())


I chose this option name somewhat arbitrarily, if it doesn't make sense it can be changed. Alternatively, we could remove Minnie-Kenny being run by sbt; Orch doesn't have it, and it's run manually in the github CI.

To confirm: Orch doesn't check for the secrets leaks commited in minniekenny.gitconfig? Is that a future ticket that can be linked in a comment?

Orch doesn't have any minnie-kenny anything of any kind; I'm not aware of any discussion around it, @davidangb do you know of anything?

I don't see any tickets in the backlog. It is used by most other services (like rawls and sam) so maybe we should add it

for my clarification: this discussion of orch is tangential to this PR, right?

yeah, if orch does not have secrets-detection, it absolutely needs it. I'd prefer to check with appsec first on the progress of their centralized/cross-repo secrets detection before committing to any work, if appsec is already handling it let's just let them.

I think it's tangential. The question came from the switch to skip testing for git-secrets via the line above that says sys.props.getOrElse("secrets.skip". From the comments, it sounded like orch was the repo/app that would add use this toggle instead of adding secrets tests?

@zarsky-broad please confirm, but my understanding is this toggle is specific to Rawls, and is called from the change in this PR in docker/install.sh.

Yeah. The old build image for Rawls didn't include git at all, so the secrets check was skipped silently. The new image has git, so the secrets check was failing here because it didn't have git-secrets. I added an optional flag to skip it that's only used in the install.sh.

The alternative is to not have sbt run the check at all, since it's explicitly run in the Github CI workflow.

Ah, I see the problem:

Orch doesn't have it, and it's run manually in the github CI.

Orch has no minnie-kenny at all. Separately, the rawls github workflow runs minnie-kenny.

I think there's a scenario here where you're comparing a Boolean to a String. My quick test in the repl shows that this can cause problems. Recommend coercing everything the same type.

kshakir

A bunch of ramblings based on scar tissue from other repos but LGTM overall

kshakir · 2021-03-09T17:55:30Z

automation/Dockerfile-tests

@@ -1,4 +1,4 @@
-FROM bigtruedata/sbt
+FROM broadinstitute/scala-baseimage:jdk11-2.12.12-1.4.7


Assuming container-based compilation is absolutely required, someday I hope this custom image gets pushed to a public GCR or anywhere else besides docker.io. We have so many tests in the terra multi-repo that are failing due to pull rate limits from docker.io these days. 😢

Or for easier patching of individual components, just use a more flexible base container like focal and then download java and the 0.016GB sbt package. While Java gets patches much slower, Scala and SBT get patches once a month. For example: sbt 1.4.8 is already out.

So:

Eventually, there's going to be Broad GCR coordinates for this image, @choover-broad is working on something for that...

This is basically just git (which I debated about) and that little sbt package on top of the base adoptopenjdk image, all the existing images were running mystery meat openjdk's, the idea was to get us on the one we're officially using.

SBT will automatically download the version of itself that you specify, so it matters a lot less what version of sbt (and scala for that matter, although it's bigger) is baked into the image.

my take is that even though sbt will automatically download whatever it needs, the entire point of adding sbt to a baseimage is to preload as many dependencies as possible at docker-build time, and avoid re-downloading those dependencies on every build of every service that uses this image. So, counting on sbt to download what it needs defeats the purpose - if we're relying on that, I could see omitting it entirely and downloading it at runtime, avoiding the maintenance of the baseimage.

kshakir · 2021-03-09T17:58:19Z

project/Merging.scala

@@ -9,6 +9,7 @@ object Merging {
    case PathList("com", "typesafe", _ @ _*) => MergeStrategy.last
    case PathList("com", "google", "auto", "value", _ @ _*) => MergeStrategy.last
    case PathList("io", "sundr", _ @ _*) => MergeStrategy.last
+    case PathList("javax", "activation", _ @ _*) => MergeStrategy.first


Interesting. These generated client libraries are including the classes, not referring to them as a dependency? I'm surprised OpenAPI doesn't have support for transitive dependencies. Is there a link to an issue that can be included as a comment?

kshakir · 2021-03-09T18:01:59Z

project/Testing.scala

@@ -55,7 +59,7 @@ object Testing {
    (testOnly in Test) := ((testOnly in Test) dependsOn validMySqlHost).evaluated,

    parallelExecution in Test := false
-  ) ++ MinnieKenny.testSettings
+  ) ++ (if (sys.props.getOrElse("secrets.skip", false) != "true") MinnieKenny.testSettings else List())


To confirm: Orch doesn't check for the secrets leaks commited in minniekenny.gitconfig? Is that a future ticket that can be linked in a comment?

davidangb

some questions/comments inline

core/src/test/scala/org/broadinstitute/dsde/rawls/RawlsTestUtils.scala

project/Dependencies.scala

davidangb · 2021-03-09T19:51:32Z

project/Merging.scala

@@ -9,6 +9,7 @@ object Merging {
    case PathList("com", "typesafe", _ @ _*) => MergeStrategy.last
    case PathList("com", "google", "auto", "value", _ @ _*) => MergeStrategy.last
    case PathList("io", "sundr", _ @ _*) => MergeStrategy.last
+    case PathList("javax", "activation", _ @ _*) => MergeStrategy.first


I much prefer using exclude rules instead of the blunt-intstrument merge strategy. Do you know where the conflicts are, and could you add excludes for those instead?

fwiw, we recently also had assembly issues in orch on javax.activation, though that was via javax.mail: broadinstitute/firecloud-orchestration#838

project/Settings.scala

zarsky-broad · 2021-03-09T21:12:18Z

project/Dependencies.scala

+  def swaggerClientExcludes(m: ModuleID): ModuleID = m.exclude("jakarta.activation", "jakarta.activation-api")
+
+  val workspaceManager = swaggerClientExcludes("bio.terra" % "workspace-manager-client" % "0.13.0-SNAPSHOT")
+  val dataRepo = swaggerClientExcludes("bio.terra" % "datarepo-client" % "1.0.44-SNAPSHOT")


@kshakir @davidangb @asingh7115 Preferable?

much better, glad this is working via excludes!

nit, optional: rename swaggerClientExcludes to e.g. excludeJakartaActivation. If it's only excluding one dependency, make it obvious in the name, similar to excludeGuavaJDK5. If it's a bundle of excludes, name it after who uses it, similar to workbenchGoogleExcludes.

lastly: do these no longer need to exclude guava-jdk5?

They don't actually include guava-jdk5, that's not included in anything vaguely recent. ~~I went with why they have the exclusion over what is being excluded, but I could see inverting it...~~ Edit: just re-read your comment, totally misread those sentences...

build.sbt

davidangb

A few more comments inline. Where I think things stand on this:

read optional comment at update to jdk 11 [AS-594] #1372 (comment), choose if you want to address it
get swatomation tests to passing

thanks for sticking with it!

zarsky-broad · 2021-03-10T20:33:47Z

Successful swatomation with firecloud-develop built from accompanying PR: https://fc-jenkins.dsp-techops.broadinstitute.org/job/swatomation-pipeline/46375/

gpolumbo-broad · 2021-03-10T21:22:42Z

project/Testing.scala

@@ -55,7 +59,7 @@ object Testing {
    (testOnly in Test) := ((testOnly in Test) dependsOn validMySqlHost).evaluated,

    parallelExecution in Test := false
-  ) ++ MinnieKenny.testSettings
+  ) ++ (if (sys.props.getOrElse("secrets.skip", false) != "true") MinnieKenny.testSettings else List())


I think there's a scenario here where you're comparing a Boolean to a String. My quick test in the repl shows that this can cause problems. Recommend coercing everything the same type.

ansingh7115

Thanks for switching to using exclusion rules!

zarsky-broad added 3 commits March 1, 2021 15:38

update to jdk 11

df335dd

fix test dockerfile image

fd4f4a8

update mockserver

237ebe5

davidangb self-requested a review March 1, 2021 22:00

zarsky-broad added 5 commits March 2, 2021 16:54

tests are on 2.12.12

4eb13a8

decrease mockserver verbosity

b5befd2

mockserver logging verbosity, try 2

7c9df33

skip minnie-kenny?

66f3133

fix for assembly issue

e96c5ec

zarsky-broad commented Mar 9, 2021

View reviewed changes

kshakir approved these changes Mar 9, 2021

View reviewed changes

zarsky-broad requested a review from ansingh7115 March 9, 2021 19:53

davidangb reviewed Mar 9, 2021

View reviewed changes

exclude instead of merging

46cee51

zarsky-broad commented Mar 9, 2021

View reviewed changes

handle lint warning

d70439b

davidangb reviewed Mar 10, 2021

View reviewed changes

build.sbt Show resolved Hide resolved

davidangb reviewed Mar 10, 2021

View reviewed changes

teeny rename

3f3d4ad

gpolumbo-broad suggested changes Mar 10, 2021

View reviewed changes

don't compare string to boolean

2f04caf

zarsky-broad requested a review from gpolumbo-broad March 10, 2021 21:26

ansingh7115 approved these changes Mar 10, 2021

View reviewed changes

gpolumbo-broad approved these changes Mar 11, 2021

View reviewed changes

davidangb approved these changes Mar 11, 2021

View reviewed changes

zarsky-broad merged commit b2972bd into develop Mar 11, 2021

zarsky-broad deleted the iz-jdk-11 branch March 11, 2021 19:17

		@@ -1,4 +1,4 @@
		FROM bigtruedata/sbt
		FROM broadinstitute/scala-baseimage:jdk11-2.12.12-1.4.7

update to jdk 11 [AS-594] #1372

update to jdk 11 [AS-594] #1372

Conversation

zarsky-broad commented Mar 1, 2021 • edited Loading

zarsky-broad commented Mar 8, 2021

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

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

kshakir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zarsky-broad Mar 9, 2021 • edited Loading

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

davidangb 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

zarsky-broad Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidangb left a comment

Choose a reason for hiding this comment

zarsky-broad commented Mar 10, 2021

Choose a reason for hiding this comment

ansingh7115 left a comment

Choose a reason for hiding this comment

zarsky-broad commented Mar 1, 2021 •

edited

Loading

zarsky-broad Mar 9, 2021 •

edited

Loading

zarsky-broad Mar 10, 2021 •

edited

Loading