-
Notifications
You must be signed in to change notification settings - Fork 23
Prevent native packager bin-compat issues #169
Prevent native packager bin-compat issues #169
Conversation
I copy/pasted the |
We'll need a release |
thanks @dwijnand for the solution in #168 (comment) |
The failure mode before this fix is that using the play sbt plugin version 2.6.21 fails to load with current sbt-reactive-app 1.6.0. So I'd simplify the test to just defining those two plugins and then loading the build (e.g test that |
Unfortunately, this PR is incomplete as it is. Trying to run |
Oh wow... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am marking this "Request changes" based on
Unfortunately, this PR is incomplete as it is.
* also simplifies the 2.6.21 scripted test to keep it in point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two fixes required. Both caused by sbt-native-packager
:
docker.DockerKeys
bin compat issueDockerAlias
|
||
// 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" | ||
} |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@@ -92,7 +93,7 @@ object SbtReactiveAppPlugin extends AutoPlugin { | |||
} | |||
} | |||
|
|||
object localImport extends docker.DockerKeys | |||
val localImport: DockerKeys = docker.DockerPlugin.autoImport |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
def index() = Action { _ => | ||
Ok("Hello, World") | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realised the easiest way to fix this is to just bump the sbt-native-package version and fix the broken API usage.
def index() = Action { _ => | ||
Ok("Hello, World") | ||
} | ||
} |
There was a problem hiding this comment.
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.
I considered that and even had that change locally but I rolled it back to not break other features. My test suite is limited and manual. |
@dwijnand I think the changes in |
@dwijnand I've made the changes in the tests you suggested:
I like it better, yeah. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH you could remove the whole build.sbt and Main.scala file too, but I guess this will do.
Fixes #168