Allow custom templates to be provided separately for each template #938
Conversation
There was a problem hiding this comment.
Thanks @ANorwell for your PR. The fix is correct, thanks for catching and fixing this.
Can you correct the failing tests? It should be just moving the files to the correct folder.
We should probably add some tests for overriding the loader functions too. If you have time, this would be awesome. You can put these all into one test and use sbt scripted test commands copy-file and delete to check if the files are picked up correctly.
| @@ -71,5 +71,5 @@ package object systemloader { | |||
| loader.toString + "/" + name | |||
|
|
|||
| private def overrideFromFile(sourceDirectory: File, loader: ServerLoader, name: String): Option[URL] = | |||
| Option(sourceDirectory / "templates" / "systemloader" / loader.toString).filter(_.exists).map(_.toURI.toURL) | |||
| Option(sourceDirectory / "templates" / "systemloader" / loader.toString / name).filter(_.exists).map(_.toURI.toURL) | |||
There was a problem hiding this comment.
Thanks for fixing this. This quite some bug. Along with 6 tests that tested the wrong implementation:
[error] (*:scripted) Failed tests:
[error] rpm / override-start-script-systemd
[error] rpm / override-start-script-systemv
[error] rpm / override-start-script-upstart
[error] debian / override-start-script-systemd
[error] debian / override-start-script-systemv
[error] debian / override-start-script-upstart
|
Hi @ANorwell, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement: |
ANorwell
force-pushed
the
vary-templates
branch
from
91175c4 to
8af0b78
Compare
|
I fixed the tests, and also added two new tests for overriding loader-scripts, one for deb and one for rpm. |
|
|
||
| val script = IO.read(extracted / "postinst") | ||
|
|
||
| assert(script.contains("# right systemd template"), s"override script wasn't picked, script is\n$script") |
There was a problem hiding this comment.
This checks that the loader-functions has be correctly inserted. I think this check is enough, even though I wouldn't my checking that the wrong systemd template isn't contained in the script.
There was a problem hiding this comment.
added such an assertion
|
Thanks again for your time and work 😊 |
This is a proposed fix for to #937.
Unfortunately, this is a breaking change to the templates directory layout, but it seems to be necessary to allow the different templates to be set independently.
Also, I'm not familiar with the sbt-native-packager codebase. This seems to be the right change, but it's possible this will have unintended consequences.