Skip to content
This repository has been archived by the owner on May 25, 2023. It is now read-only.

Prevent native packager bin-compat issues #169

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ lazy val root = (project in file("."))
IO.write(versionFile, versionSource)

Seq(versionFile)

}

addSbtPlugin("com.typesafe.sbt" % "sbt-native-packager" % Versions.nativePackager)
Expand Down
4 changes: 2 additions & 2 deletions src/main/scala/com/lightbend/rp/sbtreactiveapp/App.scala
Original file line number Diff line number Diff line change
Expand Up @@ -444,10 +444,10 @@ case object BasicApp extends DeployableApp {
val log = streams.value.log
val execCommand = dockerExecCommand.value

publishDocker(execCommand, alias.versioned, log)
publishDocker(execCommand, NativePackagerCompat.versioned(alias), log)

if (dockerUpdateLatest.value) {
publishDocker(execCommand, alias.latest, log)
publishDocker(execCommand, NativePackagerCompat.latest(alias), log)
}
}))

Expand Down
15 changes: 5 additions & 10 deletions src/main/scala/com/lightbend/rp/sbtreactiveapp/Deploy.scala
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,14 @@

package com.lightbend.rp.sbtreactiveapp

import com.lightbend.rp.sbtreactiveapp.BasicApp._
import com.lightbend.rp.sbtreactiveapp.SbtReactiveAppPlugin.localImport._
import com.lightbend.rp.sbtreactiveapp.SbtReactiveAppPlugin._
import com.typesafe.sbt.SbtNativePackager
import com.typesafe.sbt.packager.docker
import com.typesafe.sbt.packager.docker.DockerPlugin.publishLocalDocker
import com.lightbend.rp.sbtreactiveapp.SbtReactiveAppPlugin.localImport._
import com.typesafe.sbt.packager.Keys.stage
import com.typesafe.sbt.packager.docker.DockerPlugin.publishLocalDocker
import sbt.Keys._
import sbt._

import scala.collection.immutable.Seq
import com.typesafe.sbt.packager.docker.DockerSupport
import Keys._

trait DeployableApp extends App {
private val installReactiveSandbox = new java.util.concurrent.atomic.AtomicBoolean(false)
Expand All @@ -51,7 +47,6 @@ trait DeployableApp extends App {
deployMinikubePlayHttpSecretKeyValue := "dev-minikube",
deploy := {
import complete.DefaultParsers._
import scala.sys.process._

val args = spaceDelimited("<arg>").parsed
val isPlagom = Set("play", "lagom").contains(appType.value)
Expand Down Expand Up @@ -175,7 +170,7 @@ trait DeployableApp extends App {

val rpArgs =
Vector(
dockerAlias.value.versioned,
NativePackagerCompat.versioned(dockerAlias.value),
"--env",
s"JAVA_OPTS=${javaOpts.mkString(" ")}") ++
(if (bootstrapEnabled) Vector("--akka-cluster-skip-validation", "--pod-controller-replicas", deployMinikubeAkkaClusterBootstrapContactPoints.value.toString) else Vector.empty) ++
Expand All @@ -190,7 +185,7 @@ trait DeployableApp extends App {
minikubeExec.getAbsolutePath +: dockerBuildCommand.value,
log)

log.info(s"Built image ${dockerAlias.value.versioned}")
log.info(s"Built image ${NativePackagerCompat.versioned(dockerAlias.value)}")

if (reactiveSandbox) {
// FIXME: Make tiller & reactive-sandbox names configurable
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright 2017 Lightbend, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.lightbend.rp.sbtreactiveapp

import com.typesafe.sbt.packager.docker.DockerAlias

/**
* `sbt-native-packager` is not SemVer compliant and classes like `DockerAlias` break binary compatibility
* in patch releases. This object provides makes all 1.3.x versions of `sbt-native-packager` compatible.
*/
object NativePackagerCompat {

// removed from `sbt-native-packager` (https://github.com/sbt/sbt-native-packager/pull/1138/files#diff-6aa67d5df515952c884df8bad5ff2ac4L12)
def untagged(dockerAlias: DockerAlias): String =
dockerAlias.registryHost.map(_ + "/").getOrElse("") +
dockerAlias.username.map(_ + "/").getOrElse("") + dockerAlias.name

// removed from `sbt-native-packager` (https://github.com/sbt/sbt-native-packager/pull/1138/files#diff-6aa67d5df515952c884df8bad5ff2ac4L12)
def versioned(dockerAlias: DockerAlias): String = {
untagged(dockerAlias) + dockerAlias.tag.map(":" + _).getOrElse("")
}

// removed from `sbt-native-packager` (https://github.com/sbt/sbt-native-packager/pull/1138/files#diff-6aa67d5df515952c884df8bad5ff2ac4L12)
def latest(dockerAlias: DockerAlias) = s"${untagged(dockerAlias)}:latest"
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main change in the PR. There's no automated test for it, unfortunately. The issue manifests when running deploy minikube.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the part that sbt-native-packager participate in deploy minikube is just image creation, can we reproduce this by calling Docker/stage from the scripted test?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually you probably need Docker/publish to exercise alias.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good call. We can add that on a separate PR

Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package com.lightbend.rp.sbtreactiveapp
import com.typesafe.sbt.packager.Keys._
import com.typesafe.sbt.packager.archetypes.scripts.BashStartScriptPlugin
import com.typesafe.sbt.packager.docker
import com.typesafe.sbt.packager.docker.DockerKeys
import sbt.{ Def, _ }
import sbt.Keys._
import sbt.plugins.JvmPlugin
Expand Down Expand Up @@ -92,7 +93,7 @@ object SbtReactiveAppPlugin extends AutoPlugin {
}
}

object localImport extends docker.DockerKeys
val localImport: DockerKeys = docker.DockerPlugin.autoImport
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the original change. The breaking changes in docker.DockerKeys prevents user sbt's from running. This PR adds a scripted test to assert this fix kicks in.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


override def requires = SbtReactiveAppPluginAll && docker.DockerPlugin && BashStartScriptPlugin && SbtReactiveAppPluginAll

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package controllers

import javax.inject._
import play.api._
import play.api.mvc._

@Singleton
class HelloController @Inject() (cc: ControllerComponents) extends AbstractController(cc) {
def index() = Action { _ =>
Ok("Hello, World")
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This scripted test is a copy/paste/trim version of play-endpoints with a specific version of Play. I could have trimmed it more or even created a very small sbt project just mixing sbt-native-packager versions but I wanted to reproduce the issue detected on Play as closely as possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just update play-endpoints to Play 2.6.21 and keep this one minimal to the plugin conflict? Also, maybe rename this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept play-endpoints as originally implemented (depending on play's sbt-plugin 2.6.7 on purpose when I upgraded sbt-reactive-app's dependency to sbt-native-packager from 1.3.2 to 1.3.15.
But we could tell Play users that in order to use newer versions of sbt-reactive-app they have to upgrade Play too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah users should be able to always upgrade to latest bugfix version, in this case of Play and sbt-reactive-app.

13 changes: 13 additions & 0 deletions src/sbt-test/sbt-reactive-app/play-2.6.21-endpoints/build.sbt
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
name := "hello-play"
scalaVersion := "2.11.12"

libraryDependencies += guice

lazy val root = (project in file("."))
.enablePlugins(PlayScala, SbtReactiveAppPlugin)
.settings(
packageName in Docker := "hello-play",
httpIngressPorts := Seq(9000),
httpIngressPaths := Seq("/")
)

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
play.crypto.secret = whatever
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
GET / controllers.HelloController.index
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
sys.props.get("plugin.version") match {
case Some(x) => addSbtPlugin("com.lightbend.rp" % "sbt-reactive-app" % x)
case _ => sys.error("""|The system property 'plugin.version' is not defined.
|Specify this property using the scriptedLaunchOpts -D.""".stripMargin)
}

addSbtPlugin("com.typesafe.play" % "sbt-plugin" % "2.6.21")

5 changes: 5 additions & 0 deletions src/sbt-test/sbt-reactive-app/play-2.6.21-endpoints/test
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# This test only checks `sbt` runs successfuly.
# Since Play 2.6.21 there's a dependency issue between Play's sbt-plugin and sbt-reactive-app where both bring in
dwijnand marked this conversation as resolved.
Show resolved Hide resolved
# incompatible versions of sbt-native-packager. Just starting `sbt` is enough to assert the fixes for compat work.

> about