Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding rpm override script tests #891

Merged
merged 1 commit into from
Oct 7, 2016
Merged

Conversation

mitch-seymour
Copy link
Contributor

@mitch-seymour mitch-seymour commented Oct 7, 2016

I added some tests for checking the override behavior of upstart, systemd, and systemv start scripts for rpm packaging (will circle back around to debian when I have time). It seems that the start script template can only be overridden for the UpstartPlugin and SystemdPlugin. However, the SystemVPlugin does not seem to work (would like verification from someone more experienced with this plugin, who can run these new tests from their own machine).

$ cd /path/to/sbt-native-packager
$ sbt # interactive mode
> scripted rpm/override-start-script-upstart # works
> scripted rpm/override-start-script-systemd # works
> scripted rpm/override-start-script-systemv # does not work

Would someone please:

  1. Verify the systemv overriding template issue
  2. If possible, provide some guidance on a possible fix that I can include in this PR

Thanks in advance

@kardapoltsev
Copy link
Member

@kardapoltsev
Copy link
Member

  1. If possible, provide some guidance on a possible fix that I can include in this PR

I think there is a problem with a scope of sourceDirectory. Try to remove in Compile and check if it works for you.
https://github.com/mitch-seymour/sbt-native-packager/blob/master/src/main/scala/com/typesafe/sbt/packager/archetypes/systemloader/SystemVPlugin.scala#L68

@muuki88 muuki88 added the universal Zip, tar.gz, tgz and bash issues label Oct 7, 2016
@muuki88
Copy link
Contributor

muuki88 commented Oct 7, 2016

@kardapoltsev yeah, the documentation is outdated. One part gets fixed in #889

I think there is a problem with a scope of sourceDirectory. Try to remove in Compile and check if it works for you.

Very good catch!

@mackler tests are looking good. Debian would be awesome as well, but you can do this in another PR as well if you like.

@mitch-seymour
Copy link
Contributor Author

@muuki88 @kardapoltsev I fixed the scope issue in the SystemVPlugin and the override seems to work now :) Thanks for your help. I'll submit another PR with the debian tests over the weekend (need to setup a vagrant instance and do some other setup before updating those tests)

@@ -65,7 +65,7 @@ object SystemVPlugin extends AutoPlugin {
Seq(
// set the template
linuxStartScriptTemplate := linuxStartScriptUrl(
(sourceDirectory in Compile).value,
(sourceDirectory).value,
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to remove () around sourceDirectory too (:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :)

@kardapoltsev
Copy link
Member

The same issue is present in debianSettings too. Looking for a PR for debian too.
Thank you!

@kardapoltsev
Copy link
Member

LGTM now (:
Could you squash your commits please?

@mitch-seymour
Copy link
Contributor Author

commits have been squashed :)

@mackler
Copy link
Contributor

mackler commented Oct 7, 2016

@muuki88 Are you talking to me about Debian above? If so, I am unclear on what you mean and doubtful that I am competent enough to be of much use.

@muuki88
Copy link
Contributor

muuki88 commented Oct 7, 2016

Ah, sorry @mackler I was misguided by githubs auto completion ;)

@kardapoltsev squashing is not necessary anymore :) Github provides this in the UI now.
Alexey feel free to merge :)

@kardapoltsev
Copy link
Member

squashing is not necessary anymore :) Github provides this in the UI now.
I've missed this update (:

@kardapoltsev kardapoltsev merged commit 0747c4a into sbt:master Oct 7, 2016
@kardapoltsev
Copy link
Member

@mitch-seymour, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
universal Zip, tar.gz, tgz and bash issues
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants