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

Make the log file generated by application daemon configurable in RPM based SystemV #656

Merged
merged 1 commit into from
Sep 9, 2015

Conversation

fsat
Copy link
Collaborator

@fsat fsat commented Aug 31, 2015

Introduce a new settings key called rpmDaemonLogFile which will be used to configure
the name of the log file generated by application daemon in RPM based SystemV.

Make the default value of the rpmDaemonLogFile settings to be ${{app_name}}.log.

@fsat
Copy link
Collaborator Author

fsat commented Aug 31, 2015

@fsat fsat changed the title Name the log file based on the application name instead of daemon.log in RPM based SystemV WIP - DON'T MERGE - Name the log file based on the application name instead of daemon.log in RPM based SystemV Aug 31, 2015
@muuki88 muuki88 added the rpm label Aug 31, 2015
@muuki88
Copy link
Contributor

muuki88 commented Aug 31, 2015

#619 is related to this. If you could make this modifiable, then it could be configured for conductr and other things.

@fsat fsat force-pushed the rpm-systemv-log-name branch from 62cd310 to e79ffc8 Compare September 4, 2015 02:22
@fsat fsat changed the title WIP - DON'T MERGE - Name the log file based on the application name instead of daemon.log in RPM based SystemV WIP - DON'T MERGE - Make the log file generated by application daemon configurable in RPM based SystemV Sep 4, 2015
@fsat
Copy link
Collaborator Author

fsat commented Sep 4, 2015

@muuki88 - I have made this configurable with daemon.log as the default value to match existing behaviour. Updated the PR title & description to match as well.

I'm not sure how I should update the documentation since this change impacts Redhat only, not all Linux.

@@ -13,6 +13,7 @@ trait LinuxKeys {
val daemonGroup = SettingKey[String]("daemon-group", "Group to start application daemon")
val daemonGroupGid = SettingKey[Option[String]]("daemon-group-gid", "GID of daemonGroup")
val daemonShell = SettingKey[String]("daemon-shell", "Shell provided for the daemon user")
val daemonLogFile = SettingKey[String]("daemon-log-file", "Name of the log file generated by the application daemon")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be an rpm only key

Copy link
Contributor

Choose a reason for hiding this comment

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

for what reason? Not sure if other packages/system start daemons may use this as well. debian systemv for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Felix and I spoke about this and concluded that it was only needed for rpm. You can't have this choice for Debian as the log is already named with the dpkg-ing and there is no ability to change it.

@muuki88
Copy link
Contributor

muuki88 commented Sep 4, 2015

@huntc if you have no objections, feel free to merge.

@huntc
Copy link
Contributor

huntc commented Sep 5, 2015

Thanks @muuki88. I believe Felix has some more changes to push so no merge is required immediately.

@fsat fsat force-pushed the rpm-systemv-log-name branch 3 times, most recently from 8784294 to 2c6b4cc Compare September 7, 2015 00:44
@fsat fsat changed the title WIP - DON'T MERGE - Make the log file generated by application daemon configurable in RPM based SystemV Make the log file generated by application daemon configurable in RPM based SystemV Sep 7, 2015
@fsat
Copy link
Collaborator Author

fsat commented Sep 7, 2015

@huntc & @muuki88 - I have created a rpm specific settings called rpmDaemonLogFile which has ${{app_name}}.log as the default value.

This settings will allow overriding the log file name generated by the application daemon in rpm based install.

I have performed manual test for this change using the test-project. I have tested both default rpmDaemonLogFile value and overriding rpmDaemonLogFile scenario successfully.

@fsat fsat force-pushed the rpm-systemv-log-name branch from 2c6b4cc to 9527777 Compare September 7, 2015 00:54
"""|Replacements of template parameters used in linux rpm scripts.
| Default supported templates:
| rpmDaemonLogFile - name of the log file generated by application daemon
""".stripMargin
Copy link
Contributor

Choose a reason for hiding this comment

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

rpmScriptReplacements should be rpm-script-replacements

@fsat fsat changed the title Make the log file generated by application daemon configurable in RPM based SystemV WIP - DON'T MERGE - Make the log file generated by application daemon configurable in RPM based SystemV Sep 7, 2015
@fsat fsat force-pushed the rpm-systemv-log-name branch 2 times, most recently from 821ed3e to f387d55 Compare September 7, 2015 04:22
@@ -80,7 +80,11 @@ object RpmPlugin extends AutoPlugin {
target in Rpm <<= target(_ / "rpm"),
name in Rpm <<= name in Linux,
packageName in Rpm <<= packageName in Linux,
executableScriptName in Rpm <<= executableScriptName in Linux
executableScriptName in Rpm <<= executableScriptName in Linux,
rpmDaemonLogFile := s"${(packageName in Linux).value}.log",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Quoting @huntc + cc @muuki88 as commit has been updated:

For consistency I recommend using the name as per Debian packaging, and not daemon.log, unless there is some Fedora style precedence for this. This change of ours was motivated by the fact that we came upon daemon.log unexpectedly.

@muuki88 WDYT?

@fsat fsat changed the title WIP - DON'T MERGE - Make the log file generated by application daemon configurable in RPM based SystemV Make the log file generated by application daemon configurable in RPM based SystemV Sep 7, 2015
@@ -60,4 +60,15 @@ trait RpmKeys {
// Building
val rpmLint = TaskKey[Unit]("rpm-lint", "Runs rpmlint program against the genreated RPM, if available.")

val rpmDaemonLogFile = SettingKey[String]("rpm-daemon-log-file", "Name of the log file generated by application daemon")

val rpmScriptReplacements = SettingKey[Seq[(String, String)]](
Copy link
Contributor

Choose a reason for hiding this comment

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

Why introduce this new setting? linuxScriptReplacements in Rpm should be sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that rpmScriptReplacements is declared here because it is specific to RPM i.e. it isn't a script replacement destined for Linux.

If there was a key named just scriptReplacements then that'd be different...

Make sense @muuki88 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that makes sense @huntc and I like the idea for a general purpose setting scriptReplacements and scoping it to the different configurations. This will be easier if there were less settings ;)

IMHO we should not introduce a new key as removing these is much harder.

… based SystemV

Introduce a new settings key called `rpmDaemonLogFile` which will be used to configure
the name of the log file generated by application daemon in RPM based SystemV.

Make the default value of the `rpmDaemonLogFile` settings to be `${{app_name}}.log`.
@fsat fsat force-pushed the rpm-systemv-log-name branch from f387d55 to 6c07b85 Compare September 9, 2015 01:59
@fsat
Copy link
Collaborator Author

fsat commented Sep 9, 2015

@muuki88 & @huntc - changes implemented, please review.

@huntc
Copy link
Contributor

huntc commented Sep 9, 2015

LGTM

muuki88 added a commit that referenced this pull request Sep 9, 2015
Make the log file generated by application daemon configurable in RPM based SystemV
@muuki88 muuki88 merged commit 334416e into sbt:master Sep 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants