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 #472] /etc/default/<package-name> should be shell script setting envars #473

Merged
merged 3 commits into from
Feb 4, 2015

Conversation

dhardy92
Copy link
Contributor

Simply use wildly used JAVA_OPTS envars to set exec parameters.
I don't know how to tests this :/

BTW tests passes on unversal and debian

@dhardy92
Copy link
Contributor Author

note : this would break current config used

@kardapoltsev
Copy link
Member

And this doesn't allow to change $DAEMON_USER or any other $VAR in init.d script.

@dhardy92
Copy link
Contributor Author

ping ?

# if configuration files exist, source it and use JAVA_OPTS to prepend their contents to $@
[[ -f /etc/default/${{app_name}} ]] && . /etc/default/${{app_name}}
# $JAVA_OPTS used in $RUN_CMD wrapper
export JAVA_OPTS
Copy link
Contributor

Choose a reason for hiding this comment

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

will this overwrite preexisting JAVA_OPTS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it depends how JAVA_OPTS is set in /etc/default/${{app_name}} (if set)
JAVA_OPTS='toto' will overwrite,
JAVA_OPTS="toto $JAVA_OPTS" wouldn't

BTW, JAVA_OPTS seams not set before /etc/default

then I force export because I know JAVA_OPTS is required in wrapper subprocess created by start-stop-daemon (for any other envars required by java process, exporting it in /etc/default/ would be mandatory); if not, JAVA_OPTS would be limited to init.d script process only and not propagated to subprocess.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable. So the user has to make sure that preexisting JAVA_OPTS
are taken into account.

@muuki88
Copy link
Contributor

muuki88 commented Jan 29, 2015

Sorry for letting you wait. I won't have time until next week to test this.
@kardapoltsev did you test this on your systems?

@muuki88
Copy link
Contributor

muuki88 commented Jan 29, 2015

note : this would break current config used

What has the etc-default file to look like in order to work?
Do comments have to be removed?

@kardapoltsev
Copy link
Member

It works fine except that old /etc/default isn't valid. We should mention this in our docs before releasing this.

@muuki88
Copy link
Contributor

muuki88 commented Jan 29, 2015

Nice. So we need updated docs. Can you update these @dhardy92 ?
See the Developers Guide on
how to build the docs and test it.

dhardy92 added a commit to dhardy92/sbt-native-packager that referenced this pull request Jan 29, 2015
@dhardy92 dhardy92 force-pushed the fix_EtcDefaultAsShell branch from 352ccb3 to 3659743 Compare January 29, 2015 12:50
muuki88 added a commit that referenced this pull request Feb 4, 2015
[fix #472] /etc/default/<package-name> should be shell script setting envars
@muuki88 muuki88 merged commit 0e957d1 into sbt:master Feb 4, 2015
@muuki88
Copy link
Contributor

muuki88 commented Feb 4, 2015

Thanks @dhardy92 for your work :)

@knshiro
Copy link

knshiro commented Mar 11, 2015

The problem with docker is that it doesn't use an init script, and I guess that would also be causing the same problem for windows or any other system not using init.d.
If you revert only 88c7c42 at least we will have access to environment variables in the executable even if this won't change variables in the init.d script which is not the goal anyway.

@muuki88
Copy link
Contributor

muuki88 commented Mar 16, 2015

Okay. I think I got the point of this or and your request. Sorry for taking me that long :(

at least we will have access to environment variables in the executable

@knshiro
Copy link

knshiro commented Mar 16, 2015

Thank you so much! I really welcomed this change when I saw it in the changelog.

@muuki88
Copy link
Contributor

muuki88 commented Mar 16, 2015

I'm still curious about your use-case? Do you provide your own exec script? I'm not sure if relying on the start template exporting JAVA_OPTS is a good idea.

@knshiro
Copy link

knshiro commented Mar 16, 2015

My use case is that I'm using https://github.com/gilt/sbt-newrelic and I don't want to pack the API key in the package I'm making. The problem is that the only way I can specify the key to the agent, except that writing a whole new config file, is to provide an environment variable.

@muuki88
Copy link
Contributor

muuki88 commented Mar 16, 2015

@knshiro
Copy link

knshiro commented Mar 16, 2015

I need to use an environment variable, not a Java property.

@muuki88
Copy link
Contributor

muuki88 commented Mar 17, 2015

except that writing a whole new config file, is to provide an environment variable.

Exactly this is JAVA_OPTS for. This pr didn't include env variables, it just treated the conf file as a bash script.

You can use JAVA_OPTS to configure your Java options. E.g. ( not tested as I'm currently traveling )

export JAVA_OPTS=-Dnewrelic.api.key=123abc
./bin/your-app

@knshiro
Copy link

knshiro commented Mar 17, 2015

I'm using the server archetype so I'm counting on init.d to launch the application so I can't add environment variables from the shell.
The whole process I have right now is that I'm packaging a deb and use packer (with sbt-packer) to create an AMI and then use cloudinit configuration to add specific parameters. Right now what I'm doing is that in this cloudinit I write a file in /etc/default/my_app_env and I added a line in the bin script to source this file. With the current logic you can only write JAVA_OPTS in the /etc/default/my_app file and Newreilic does not use java properties so I had to add manually this line in the bash script thanks to bashScriptExtraDefines:

bashScriptExtraDefines <++= (bashScriptConfigLocation in Universal) map { _.map { config =>
    val envConfig = config + "_env"
      "[[ -f '" + envConfig +"' ]] && source " + envConfig 
    }.toSeq

/etc/default/my_app_env

export NEW_RELIC_LICENSE_KEY=123abc

@muuki88
Copy link
Contributor

muuki88 commented Mar 17, 2015

This is great! First thing, this small piece of SBT code would be the way this additional feature should be integrated.

I wasn't really happy about breaking the old configuration format as it provides a more convenient way to specify Java and application arguments.

However sourcing a configuration file that is not included in the package before configure/running the app is a good thing to have.

Implementation Suggestion

I would favor a new setting like bashScriptEnvConfigLocation: Option[String].

In the app archetype this would look like

bashScriptExtraDefines <++= (bashScriptEnvConfigLocation in Universal) map { _.map { config =>
      "[[ -f '" + config +"' ]] && source " + config 
    }.toSeq

WDYT?
Would you like to provide this as a PR?

@dhardy92
Copy link
Contributor Author

Actually this file is suppose to be /etc/default/ in debian ( or /etc/sysconfig/ in rpm ) in standard way and this should be confusing.

The current file should be renamed as "/etc//init.conf" where you can do what you want (or other proper filename) with non standard way to handle params used properly by wrappers
and give back /etc/default to what is should be (sourced by init scripts).

@kardapoltsev
Copy link
Member

+1

@knshiro
Copy link

knshiro commented Mar 17, 2015

I made the PR anyway. You can modify it further as you see fit.

@muuki88
Copy link
Contributor

muuki88 commented Mar 17, 2015

PR looks good. We should swap the names of the conf files in a second pr according to @dhardy92 suggestions. @kardapoltsev do you have time for this?

@kardapoltsev
Copy link
Member

@muuki88 I have time but can't access my machine today. I could only look throw code.

@muuki88
Copy link
Contributor

muuki88 commented Mar 18, 2015

@kardapoltsev No problem ;) I gave it a bit more thought and maybe I add this change to #510 as I have to rebase it anyway. My suggestion would be

setting location description
bashScriptEnvConfigLocation /etc/default/appname sourced by init scripts
bashScriptConfigLocation app_home/conf/application.ini parsed by bash script

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.

4 participants