-
Notifications
You must be signed in to change notification settings - Fork 455
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
Worker config updates to make path to jave.exe configurable #3210
Conversation
Sample worker.config.json with AppServiceEnvironment profile
PR Azure/azure-functions-java-worker#151 updates worker.config.json for java worker |
31f8148
to
3c8e082
Compare
Updated to Java worker. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if it would save us all some time (and avoid migration issues) if this was targeting the hosting branch instead. We're about to merge that in. What do you think?
@brettsam , thoughts?
Yes! and in fact, there's some issues with worker config today in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of nit comments
@@ -341,10 +341,14 @@ internal void StartProcess(string workerId, Process process) | |||
{ | |||
_logger.LogWarning(e.Data); | |||
} | |||
else | |||
else if (e.Data.IndexOf("error", StringComparison.OrdinalIgnoreCase) > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this > -1
?
public const string JavaLanguageWorkerName = "java"; | ||
// Profiles | ||
public const string WorkerDescriptionProfiles = "profiles"; | ||
public const string AppServiceEnvDescription = "AppServiceEnvironment"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be something like WorkerProfileNameAppServiceEnvironment
?
@brettsam , just had a quick chat with @pragnagopa . Since we haven't changed that model for language workers in the hosting branch yet, I don't think what she is adding expands what we'd have to migrate too much, so it should be fine to go into |
Fixes #3187, Fixes #3048