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

SystemD services now source /etc/default/{{app_name}} (resolves #737) #745

Merged
merged 1 commit into from Feb 23, 2016

Conversation

timcharper
Copy link
Contributor

This change modifies the SystemD template to include the packaged
/etc/default environment settings file. Since a sourced SystemD
environment file is not the same thing as a bourne shell source script,
a different template is used.

Those deploying their package to multiple platforms can also specify
different /etc/default templates by suffixing -systemd to their name,
ie: src/templates/etc-default-systemd

@timcharper
Copy link
Contributor Author

@muuki88, this change is done and tested (lamentably, only under Debian Linux). What are your thoughts on my approach? I'd like to get some feedback on it before recommending it for merge.

@@ -9,3 +9,4 @@ project/project
log/
target/
.cache
.ensime*
Copy link
Contributor

Choose a reason for hiding this comment

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

pure personal interest: what editor do you use with ensime?

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to recommend emacs + evil (:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's it! Emacs and evil :)

@muuki88
Copy link
Contributor

muuki88 commented Feb 21, 2016

I like the implementation a lot. We add further customization without touching the existing solution. If you add one test for the customization and some documentation this PR is from my perspective ready to merge

/* etcDefaultConfig is dependent on serverLoading (systemd, systemv, etc.),
* and is therefore distro specific. As such, these settings cannot be defined
* in the global config scope. */
private val etcDefaultConfig: Seq[Setting[_]] = Seq(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand this; private[this] is not smaller than private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scala> trait A { private val a = 1 }
defined trait A

scala> object B extends A { println(a) }
<console>:11: error: not found: value a
       object B extends A { println(a) }
                                    ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What ? Codacy is clearly wrong here. Protected is visible by subclasses. Private is not. It has always been that way.

This change modifies the SystemD template to include the packaged
/etc/default environment settings file. Since a sourced SystemD
environment file is not the same thing as a bourne shell source script,
a different template is used.

Those deploying their package to multiple platforms can also specify
different /etc/default templates by suffixing `-systemd` to their name,
ie: src/templates/etc-default-systemd
@timcharper
Copy link
Contributor Author

@muuki88 Quick ping to say I've done as you've requested and everything's green again.

muuki88 added a commit that referenced this pull request Feb 23, 2016
SystemD services now source /etc/default/{{app_name}} (resolves #737)
@muuki88 muuki88 merged commit e32a059 into sbt:master Feb 23, 2016
@muuki88
Copy link
Contributor

muuki88 commented Feb 23, 2016

Awesome. Thanks a lot, I will try to release an RC

@timcharper
Copy link
Contributor Author

\o/

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

Successfully merging this pull request may close these issues.

None yet

3 participants