init.sh uses go-java-launcher instead of Gradle startup scripts #85
Conversation
uschi2000
force-pushed
the
rfink/go-launcher
branch
from
450c504
to
cbc7a39
Compare
uschi2000
force-pushed
the
rfink/go-launcher
branch
from
cbc7a39
to
c4d43f1
Compare
| }) << { | ||
| // TODO(rfink) Don't check these in, but fetch them from a published artifact? | ||
| ["javalauncher-linux-amd64", "javalauncher-darwin-amd64"].each { file -> | ||
| def dest = Paths.get("${project.buildDir}/scripts/${file}") |
There was a problem hiding this comment.
This should probably end up in var/conf. config.sh was in the immutable section because it was a script (and thus vulnerable to tampering).
There was a problem hiding this comment.
Sorry, this should've been on the launch config file, not the launchers.
There was a problem hiding this comment.
double-checking the reasoning here: the file is really part of the binary and is not intended to be edited by anyone directly. I think it's more correct to put it in bin/. as it stands, the stuff in the var/conf directory of the distribution is exactly the contents of the var/conf directory of the source, and conversely everything in bin/ is generated; I like that separation.
There was a problem hiding this comment.
I think the launch config options need to be editable (say to safely add env variables via deployment software). Users definitely need to be able to modify heap settings as well.
Fine putting it in var/launch or some other place, but don't think it can go in the service dir, since by mandate that's not editable.
Sent from my iPhone
On Jul 6, 2016, at 10:40 AM, Robert Fink <notifications@github.commailto:notifications@github.com> wrote:
In src/main/groovy/com/palantir/gradle/javadist/JavaDistributionPlugin.groovyhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_palantir_gradle-2Djava-2Ddistribution_pull_85-23discussion-5Fr69776704&d=DQMCaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=Vp81aOxWZvuVgVK_wp-VF3pIYG92B19LcCw6XKeYC0U&m=WP78GPbLtpuvYIwP3MU0cTu5Ks7YcCYBI_HSG6YaIEs&s=cGdCMvQ6AbO6HTPAPgynXiiWdsDzpN_Z1s3bkMiwn_c&e=:
@@ -58,19 +59,37 @@ class JavaDistributionPlugin implements Plugin {
}
}
Task copyLauncherBinaries = project.tasks.create('copyLauncherBinaries', {group = GROUP_NAMEdescription = "Creates go-java-launcher binaries."}) << {// TODO(rfink) Don't check these in, but fetch them from a published artifact?["javalauncher-linux-amd64", "javalauncher-darwin-amd64"].each { file ->def dest = Paths.get("${project.buildDir}/scripts/${file}")
double-checking the reasoning here: the file is really part of the binary and is not intended to be edited by anyone directly. I think it's more correct to put it in bin/. as it stands, the stuff in the var/conf directory of the distribution is exactly the contents of the var/conf directory of the source, and conversely everything in bin/ is generated; I like that separation.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_palantir_gradle-2Djava-2Ddistribution_pull_85_files_c4d43f123e5aa09f0f0ef95acf96f33ccb0ec982-23r69776704&d=DQMCaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=Vp81aOxWZvuVgVK_wp-VF3pIYG92B19LcCw6XKeYC0U&m=WP78GPbLtpuvYIwP3MU0cTu5Ks7YcCYBI_HSG6YaIEs&s=tiIBWzyqjE8ir0YfbzNT6iaPiTDC9-9Ya5IEoa77hoY&e=, or mute the threadhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe_AAp3Q6rhZBB23sx6ldISZouRbx8Y-5FvSIks5qS-2DihgaJpZM4JFzUs&d=DQMCaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=Vp81aOxWZvuVgVK_wp-VF3pIYG92B19LcCw6XKeYC0U&m=WP78GPbLtpuvYIwP3MU0cTu5Ks7YcCYBI_HSG6YaIEs&s=UDAyzLjQMBRRRpIpHHJ33mkzPG54Wt8F2c3MsDiS5-w&e=.
There was a problem hiding this comment.
var/launch/launcher,yml, actually
There was a problem hiding this comment.
one more thought here, we can also split the configs between things that should not be editable, saved in service/bin and things that should be editable (such as java options) in var/launcher.
There was a problem hiding this comment.
main class and classpath probably should be static, for instance (the classpath thing might be debatable)
There was a problem hiding this comment.
hmm, yeah maybe. not sure this is super high priority though...
There was a problem hiding this comment.
Nope, fine leaving as a singular conf file.
|
Changes as per comments above. Left to be done: check.sh needs to call javalauncher instead of deprecated shell script |
|
Yep, calling javalauncher with the args provided from the config should be okay. Also fine adding "healthcheck" or "health" as an option in init.sh and having check.sh just call init.sh with that explicit arg. We should probably also clean up init.sh, it's gotten gnarly/bad. |
|
…but can wait on the init.sh for another PR, if you like. |
| init.sh # daemonizing script | ||
| config.sh # customized environment vars | ||
| [service-name] # Bash start script | ||
| [service-name.bat] # Windows start script |
There was a problem hiding this comment.
should we tag these as unused?
There was a problem hiding this comment.
already done in the section below
| } | ||
| } | ||
|
|
||
| private static List<String> getSlsv2RelativeClasspath(FileCollection files) { |
There was a problem hiding this comment.
sorry, one last nit, let's call this getServiceLibDirectory.
One interesting thing we may need to deal with here is custom classpath entries which may not necessarily live in lib, not sure how best to deal with that, though.
|
ok, I think I addresses all things |
|
binaries are still checked in, blocked on repo creation |
| .toFile() | ||
| .setExecutable(true) | ||
| } | ||
|
|
||
| // TODO(rfink) The check script should go away, or at least it should not call the Gradle start script |
|
lgtm |
| SERVICE="@serviceName@" | ||
| PIDFILE="var/run/$SERVICE.pid" | ||
| LAUNCHER_CONFIG="var/launch/launcher.yml" | ||
| LAUNCHER_CHECK_CONFIG="var/launch/launcher-check.yml" |
There was a problem hiding this comment.
I'm not sure I see where this gets generated
There was a problem hiding this comment.
|
Where is the source code for those checked-in Go binaries? |
|
replied offline On Wed, Jul 6, 2016 at 3:27 PM Andrew Ash notifications@github.com wrote:
|
"Works."
Todos: