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

feat: Spawn offline process with sls offline command #214

Merged
merged 1 commit into from
Jul 25, 2019

Conversation

tbarlow12
Copy link
Collaborator

  • sls offline builds Azure Functions files and spawns func host start in the same shell
  • sls offline start spawns func host start in the same shell
  • BaseService has spawn function that spawns and waits for child process in shell (routes stdout and stderr to internal logger)
  • Using mock-spawn library to mock spawned child processes
  • Add invocation commands to config constants

Resolves [AB#569]

@coveralls
Copy link

coveralls commented Jul 24, 2019

Coverage Status

Coverage decreased (-0.2%) to 84.211% when pulling 797a444 on tabarlow/spawn-child into 844ffb5 on dev.

Copy link
Member

@wbreza wbreza left a comment

Choose a reason for hiding this comment

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

Looks good overall! Added some minor feedback.

src/services/baseService.ts Outdated Show resolved Hide resolved
@tbarlow12 tbarlow12 force-pushed the tabarlow/spawn-child branch 2 times, most recently from 4e96afd to f836b34 Compare July 24, 2019 19:13
Copy link
Collaborator Author

@tbarlow12 tbarlow12 left a comment

Choose a reason for hiding this comment

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

@wbreza Added some additional functionality for the console logging after looking into sls core. Wanna take another peek?

Copy link
Collaborator

@PIC123 PIC123 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

*/
private spawn(command: string, args?: string[], env?: any): Promise<void> {
env = {
...this.serverless.service.provider["environment"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

💯

@pjlittle
Copy link
Collaborator

nice work, looks great 👍

Move spawn to offlineService

Add styles to logging

Fix tests with log statements
@wbreza wbreza merged commit 77553ae into dev Jul 25, 2019
}
this.log(`Spawning process '${command} ${args.join(" ")}'`);
return new Promise((resolve, reject) => {
const childProcess = spawn(command, args, {env});

Choose a reason for hiding this comment

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

I don't know if it has been made on purpose (if yes, that requires maybe some documentation), but passing the env variable here as an option is causing the default value process.env to not be used (and more particularly the PATH in it). Taking the PATH from process.env if it hasn't been set as an additional environment variable could be a solution.

https://nodejs.org/docs/latest-v10.x/api/child_process.html#child_process_child_process_spawn_command_args_options

@tbarlow12 tbarlow12 deleted the tabarlow/spawn-child branch August 6, 2019 14:58
tbarlow12 added a commit that referenced this pull request Sep 13, 2019
sls offline builds Azure Functions files and spawns func host start in the same shell
  sls offline start spawns func host start in the same shell
  BaseService has spawn function that spawns and waits for child process in shell (routes stdout and stderr to internal logger)
  Using mock-spawn library to mock spawned child processes
  Add invocation commands to config constants
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.

None yet

6 participants