-
Notifications
You must be signed in to change notification settings - Fork 54
Change current directory on environment reload request #522
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
Conversation
…e-worker-protobuf. Tag: v1.3.8-protofile. Commit: 8db40c8
|
/cc @pragnagopa |
src/RequestProcessor.cs
Outdated
|
|
||
| var setCurrentDirMessage = string.Format(PowerShellWorkerStrings.SettingCurrentDirectory, environmentReloadRequest.FunctionAppDirectory); | ||
| rpcLogger.Log(isUserOnlyLog: false, LogLevel.Trace, setCurrentDirMessage); | ||
| Directory.SetCurrentDirectory(environmentReloadRequest.FunctionAppDirectory); |
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 we check that the directory exists before setting it? I remember that in the past for proxy call the working directory could be null.
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.
Checking for null: yes, thank you, done.
Checking that the directory exists: I don't know if we have a better way of handling the situation when it does not exist. Perhaps the best we can do is trust the host and fail if it does not exist - this is what NodeJs and Python workers also do.
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.
Yes. Trusting the host is OK. Any error handling should be done in the host for this.
Francisco-Gamino
left a comment
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.
One minor comment.
Also, I don't remember if for updating protobuff files the we need a separate PR just for that, so please follow-up with @pragnagopa. Other than that, LGTM.
pragnagopa
left a comment
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.
Thanks. LGTM. Added a minor comment on tests.
* Updated subtree from https://github.com/azure/azure-functions-language-worker-protobuf. Tag: v1.3.8-protofile. Commit: 8db40c8 * Set current directory on environment reload request
* Updated subtree from https://github.com/azure/azure-functions-language-worker-protobuf. Tag: v1.3.8-protofile. Commit: 8db40c8 * Set current directory on environment reload request
* Updated subtree from https://github.com/azure/azure-functions-language-worker-protobuf. Tag: v1.3.8-protofile. Commit: 8db40c8 * Set current directory on environment reload request
* Updated subtree from https://github.com/azure/azure-functions-language-worker-protobuf. Tag: v1.3.8-protofile. Commit: 8db40c8 * Set current directory on environment reload request
* Updated subtree from https://github.com/azure/azure-functions-language-worker-protobuf. Tag: v1.3.8-protofile. Commit: 8db40c8 * Set current directory on environment reload request
Resolves issue #521