Use spawn instead of exec for executing commands

[zimme]

This change will resolve a bug that prevents stdout/stderr from being logged in case of a command execution resulting in error.

The mock implementation needs to be fixed as tests are failing. However I don't have time to digg into that currently.

I thought I'd at least post the code so we can discuss if this is the correct way to go about resolving the issue at hand.

[benjie]

LGTM :+1:

[zimme]

• Rebased on main
• Updated the spawn promise wrapper to skip the child.on("error", ...) part
• Switched mockedExec for mockedSpawn and updated mock implementations

[zimme]

I'm not sure if we need to handle more cases then just "close" in the spawn promise wrapper and if we should resolve the promise even if there's a non-zero code.

async function spawn(
  command: string,
  options: SpawnOptions,
): Promise<ChildProcess> {
  return new Promise((resolve, reject) => {
    const child = rawSpawn(command, [], options);

    child.on("close", code => {
      if (code === 0) {
        resolve(child);
      } else {
        reject(new Error(`Command failed with code ${code}`));
      }
    });
  });
}

But I'm thinking that we've configured spawn to inherit the stdio so any error thing should log to stderr and if there's an error the code should become non-zero, so then the question becomes should we throw an error to stop migrate or just resolve it so migrate continues?

I haven't tested these new changes on my own project yet. I'll get back with an update after that to see what makes sense.

I started this becuase I run schemalint in an after current hook and when it gave errors they were just swallowed.

[zimme]

So for me in my project this works as expected.

error

[2022-11-21T11:17:15.385Z]: Running current.sql
schema-lint
Connecting to undefined on undefined
app_public.cars: error require-primary-key : The table app_public.cars does not have a primary key defined";

Suggested fix
ALTER TABLE "app_public.cars" ADD PRIMARY KEY (<primary key column or columns>)";

🛑 Error occurred whilst processing migration
    Error: Command failed with code 1
        at ChildProcess.<anonymous> (/Users/zimme/Development/graphile/migrate/dist/actions.js:20:24)
        at ChildProcess.emit (node:events:513:28)
        at maybeClose (node:internal/child_process:1100:16)
        at Process.ChildProcess._handle.onexit (node:internal/child_process:304:5)

not error

[2022-11-21T11:20:35.429Z]: Running current.sql
schema-lint
Connecting to undefined on undefined
No issues detected

I don't know if we need more testing, etc.

But for my use case I'm happy.

[zimme]

Thanks for the review and new solution even 😄 Completely understandable, I've seen how busy you've been.

[benjie]

Wow, speedy response! Yeah, having now better understood the underlying error I figured a smaller fix would be safer 👍

[benjie]

Fix is out in 1.4.1