PDMLab / docker-compose

Manage Docker-Compose via Node.js
https://pdmlab.github.io/docker-compose/
MIT License
199 stars 78 forks source link

docker-compose fails when path to configuration includes spaces #129

Closed noahtallen closed 3 years ago

noahtallen commented 3 years ago

Noticed at: https://github.com/WordPress/gutenberg/issues/28680#issue-799846994

When we access dockerCompose commands, we often pass the path to the config file: dockerCompos.run( 'cli', 'wp db check', { config: pathToConfig } );

However, when the path includes spaces (like C:\Users\User Name\path\to\docker-compose-config.yml), it breaks the command.

I believe this option needs to be quoted somewhere near here for it to work correctly in this context: https://github.com/PDMLab/docker-compose/blob/483e6827adc7705602d31ff57a826565ab4fec82/src/index.ts#L80

Steveb-p commented 3 years ago

I've tried reproducing this on linux with the following test:

test('ensure container command executed with --workdir command option with space', async (): Promise<void> => {
  await compose.down({ cwd: path.join(__dirname), log: logOutput, config: 'directory with space/docker-compose.yml' });
  const result = await compose.run('some-service', 'pwd', {
    cwd: path.join(__dirname),
    log: true,
    config: 'directory with space/docker-compose.yml',
    composeOptions: [ '--verbose' ],

    // Alpine has "/" as default
    commandOptions: [ '--workdir', '/home/root' ]
  });

  expect(result.out).toBe('/home/root\n');

  await compose.down({ cwd: path.join(__dirname), log: logOutput, config: 'directory with space/docker-compose.yml' });
});

Unfortunately, it seems that in this case it works as intended, i.e. I receive /home/root as response.

It's possible that this is an issue with Windows docker only. I don't have a virtual machine on hand at the moment, so I'll have to set one up when I'm free - ideally, if you could confirm that this test fails on the reported environment, I'd immensely grateful.

Also, cool to see that Gutenberg actually uses this library :smile:

EDIT: @noahtallen I've looked up the original issue. Are you sure that this even happens in the run.js script that is mentioned? FWIW process.spawn (coming from Node.js) should correctly handle argument and option escaping, I think.

AlexZeitler commented 3 years ago

@Steveb-p Just installed Docker + Node in my Windows VM and ran your test:

image

AlexZeitler commented 3 years ago

@noahtallen Can you provide a repro, please?

Which Docker/Docker Compose Version do you use on Windows?

Are you using PowerShell (Core) to run your containers or WSL2?

noahtallen commented 3 years ago

This is my bad, sorry! There was a direct call to exec( docker-compose ) in our program which I was unaware of. It was not quoting the path correctly, hence the error. Definitely on our end, not here!