Skip to content

Add support to reload environment variables #20

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

Merged
merged 4 commits into from
Nov 30, 2018
Merged

Conversation

pragnagopa
Copy link
Member

@pragnagopa pragnagopa commented Nov 27, 2018

Added new message to support reloading of environment variables. This message is specifically used during specialization when functions runtime starts a language worker in place holder mode and then assigns to an app at a later point in time.

@pragnagopa pragnagopa requested a review from mhoeger November 27, 2018 17:42
@mhoeger
Copy link
Contributor

mhoeger commented Nov 27, 2018

@pragnagopa - could you add some context for this PR in the description?

@@ -180,6 +184,16 @@ message WorkerStatusRequest{
message WorkerStatusResponse {
}

message FunctionEnvironmentLoadRequest {
// Environment variables from the current process
map<string, string> environment_variables = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can an environment variable's value ever be null or empty? languages like javascript will buffer overflow if so :|

Copy link
Member Author

Choose a reason for hiding this comment

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

we can control this in host.

@pragnagopa pragnagopa added this to the Functions Sprint 38 milestone Nov 29, 2018
@@ -65,6 +65,10 @@ message StreamingMessage {

// Worker logs a message back to the host
RpcLog rpc_log = 2;

FunctionEnvironmentLoadRequest function_environment_load_request = 25;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call it something like "FunctionEnvironmentReloadRequest", since environment variables will be there by default for most cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense. will update

@@ -180,6 +184,16 @@ message WorkerStatusRequest{
message WorkerStatusResponse {
}

message FunctionEnvironmentLoadRequest {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you might have missed renaming the message names!

Copy link
Member Author

Choose a reason for hiding this comment

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

duh..yes. thanks!

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

Successfully merging this pull request may close these issues.

2 participants