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

fix SystemV init script template for debian packaging #697

Merged
merged 2 commits into from Nov 19, 2015

Conversation

yanns
Copy link
Contributor

@yanns yanns commented Nov 13, 2015

For processes that manage their own pid file, they should define the PIDFILE variable (sourced either through /etc/default/… or /etc/sysconfig/…). Otherwise, if undefined, this script will capture the pid and write it to /var/run/${{app_name}}/running.pid.

This commit is related to 45bc5db that implements the same logic for rpm packaging

For processes that manage their own pid file, they should define the `PIDFILE` variable (sourced either through `/etc/default/…` or `/etc/sysconfig/…`). Otherwise, if undefined, this script will capture the pid and write it to `/var/run/${{app_name}}/running.pid`.

This commit is related to sbt@45bc5db that implements the same logic for rpm packaging
@kardapoltsev
Copy link
Member

One note about stopping daemon. We probably shouldn't remove PIDFILE in case when it's managed by app itselt.

@yanns
Copy link
Contributor Author

yanns commented Nov 13, 2015

@kardapoltsev good point.
But for a play application for example, the stop script must know the pid to send a kill signal to the application. I think we need the PIDFILE for the stop.

@kardapoltsev
Copy link
Member

We need PIDFILE for stopping service but I think that it shouldn't be removed by start script. Play application removes its own pid file as I remember.

@muuki88 muuki88 added debian universal Zip, tar.gz, tgz and bash issues labels Nov 14, 2015
@muuki88
Copy link
Contributor

muuki88 commented Nov 14, 2015

@yanns code looks good to me. Things I would like to see before merging this:

  • Adding documentation in the play section and the server configuration
  • a log-daemon msg which path is used ( pid file managed by app or pid file managed by start-stop daemon )

Thanks for making the SystemV script more robust :)

@yanns
Copy link
Contributor Author

yanns commented Nov 14, 2015

@kardapoltsev I now understood what you mean. You're right. I'll change that.

@yanns
Copy link
Contributor Author

yanns commented Nov 14, 2015

@muuki88 the play section and the server configuration already contain the needed information.
It was implemented in the rpm start script but not in the debian one (see the linked commit)

OK for the msg. I'll add that.

@muuki88
Copy link
Contributor

muuki88 commented Nov 14, 2015

@yanns yes, it's added to the docs, but it states that this is for rpm / systemv, where now you would have to add it for debian as well. Which will again create some confusion for play users.

@yanns
Copy link
Contributor Author

yanns commented Nov 14, 2015

@muuki88 yes you're right, I oversaw that in the play doc.
But in the server doc, it's only mentioning systemv. There is here nothing to change I guess.

@yanns
Copy link
Contributor Author

yanns commented Nov 14, 2015

I tested writing the msg and I am not sure we should do it:

$  /etc/init.d/my-app start
 * Starting my-app
using PID file '/var/run/myapp/play.pid' managed by application
                                                                                   [ OK ]

It breaks the output.

@muuki88
Copy link
Contributor

muuki88 commented Nov 14, 2015

ewy. Okay, we leave it as is.

@yanns
Copy link
Contributor Author

yanns commented Nov 15, 2015

@kardapoltsev @muuki88 please review the new version that contain changes according to your feedbacks.

@muuki88
Copy link
Contributor

muuki88 commented Nov 15, 2015

LGTM

@kardapoltsev
Copy link
Member

LGTM too except that @muuki88 mentioned log message about pid file management.

a log-daemon msg which path is used ( pid file managed by app or pid file managed by start-stop daemon )

@yanns
Copy link
Contributor Author

yanns commented Nov 19, 2015

@kardapoltsev please have a look at the previous discussions.
I tried to add the msg and it breaks the output. With @muuki88 we decided not to make it.

@kardapoltsev
Copy link
Member

Excuse me, I've missed it. Thanks for your contributions!

kardapoltsev added a commit that referenced this pull request Nov 19, 2015
fix SystemV init script template for debian packaging
@kardapoltsev kardapoltsev merged commit 301dea4 into sbt:master Nov 19, 2015
@yanns yanns deleted the debian_start_script_PIDFILE branch November 19, 2015 10:47
@yanns
Copy link
Contributor Author

yanns commented Nov 19, 2015

cool thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants