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

Sysvinit script for debian package #85

Merged
merged 4 commits into from
Nov 25, 2013
Merged

Conversation

kardapoltsev
Copy link
Member

No description provided.

@lightbend-cla-validator

@kardapoltsev Please sign the latest Typesafe Contributors License Agreement so that we can review and hopefully merge this Pull Request:
http://typesafe.com/contribute/cla

@kardapoltsev
Copy link
Member Author

I did sign the CLA.

@jsuereth
Copy link
Member

@kardapoltsev I think it's a new variant of the license or something (says which country/state law it's under which is important for some reason). If not, I apologize, we're experimenting with github pull request hooks and automation.

THanks so much for the patch! I'll dig in and review now. Looking forward to seeing more great work.

@@ -1 +1 @@
sbt.version=0.12.4
sbt.version=0.13.0
Copy link
Member

Choose a reason for hiding this comment

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

We can't actually upgrade yet, as we still support 0.12 for at least another 6 months.

@jsuereth
Copy link
Member

Code looks great, but raises some architectural questions....

I think I want to drive System V vs. upstart via flag, like:

serverLoading in Debian := ServerLoader.UpStart

serverLoading in Rpm := ServerLoader.SystemV

This does make the change a bit hairer though, as driving settings off values of other settings can make the code a bit convoluted....

What do you think?

@kardapoltsev
Copy link
Member Author

Sure! I could reimplement this part.
Thanks for your review!

Regards, Alexey
On Nov 22, 2013 6:41 PM, "Josh Suereth" notifications@github.com wrote:

Code looks great, but raises some architectural questions....

I think I want to drive System V vs. upstart via flag, like:

serverLoading in Debian := ServerLoader.UpStart
serverLoading in Rpm := ServerLoader.SystemV

This does make the change a bit hairer though, as driving settings off
values of other settings can make the code a bit convoluted....

What do you think?


Reply to this email directly or view it on GitHubhttps://github.com//pull/85#issuecomment-29069573
.

@kardapoltsev
Copy link
Member Author

Should I create a new pull request with this changes? I'm new in contributing to public repos.

@jsuereth
Copy link
Member

Up to you.

What we generally recommend is if you're going to rewrite history on a branch is to name it something with "Work in Progress" or WIP in the name. Otherwise, a new branch can be easier.

In this case, it's up to you what you wish to do.

@kardapoltsev
Copy link
Member Author

I'm done with settings for start system, could you review this version?

@jsuereth
Copy link
Member

This looks great! Thanks much for the changes. The only thing I'd like to see now is making this also work for Init.d RPM scripts :) I think that's a follow up PR.

Thanks much for the work. It looks great.

jsuereth added a commit that referenced this pull request Nov 25, 2013
Sysvinit script for debian package
@jsuereth jsuereth merged commit 8843f58 into sbt:master Nov 25, 2013
@kardapoltsev
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants