Support Docker Buildkit about docker-compose HOT 16 CLOSED

@AleF83 @sbalay Would it make sense to you to have a useDockerBuildKit option at IDockerComposeBuildOptions which handles this?

Would you be willing to send a PR for this?

from docker-compose.

Makes sense! I'll send the PR.

from docker-compose.

There are multiple reasons why you would want to avoid that:

1. BC break (for example DOCKER_COMPOSE_PROJECT env would be merged into command, which in turn would spawn containers with different names)
2. Possible attack vector by someone stealing some of your confidential env variables. Imagine an app allowing you to start up docker containers using this lib and your database credentials get leaked.
3. Inability to control the reverse behavior (which would make it necessary to introduce an option anyway, in case you do not want to merge your env params).

In general cases like this should be rather documented instead of trying to implicitly do something for an user. I'd go for explicit env variable merging option.

from docker-compose.

I'm not sure if I understood you correctly, but just want to clarify my previous comment:

I think that the best way to deal with this issue, is not doing anything. I would not change the code to add a new option (mergeEnv) and I would not merge process.env implicitly because of the reasons you mentioned before.

Instead, I suggest just adding a note in the readme clarifying that the env option provided to this library is handled as it is to child_process, and explaining how child_process works with it.

So, keep the implementation as it is, and just improve the docs a little bit.

from docker-compose.

@AleF83 I had the same issue. The problem here is that when you specify the env param, child_process does not merge it with the current env variables, so you have to do it manually:

const options: dockerCompose.IDockerComposeBuildOptions = {
  cwd,
  env: {
    COMPOSE_DOCKER_CLI_BUILD: '1',
    DOCKER_BUILDKIT: '1',
    ...process.env,
  },
  log: true,
};

Doing so should make your script work.

from docker-compose.

Isn't it not only Docker Buildkit specific? The merging of environment variables I mean? We could just have a mergeEnv / mergeProcessEnv option instead.

from docker-compose.

What about always merging options.env with process.env without even having a mergeEnv param? Sounds like a reasonable behaviour to adopt it by default.

Then we would not need new options

from docker-compose.

Valid reasons 😅

Then, I could add the mergeEnv and send PR if you think it's the best alternative... but I feel like we are trying to workaround a design decision in nodeJS (discussion in this thread).

If I were you, I would just add a note in the readme explaining how things work if you specify the env option (it is not merged to process.env by default) when using this library and maybe also add recommendations on how to deal with that. I would't add any new option unless child_process also adds it in the future.

Thoughts?

from docker-compose.

I'm pretty sure that decision was consciously made - and you can see that the thread is already closed. I don't think it's changing anytime soon. If not for anything then for reasons above.

Workaround is simple enough and explicit. Just pass your process.env into env option. I understand that docker-compose (this lib) is actually a wrapper around child_process module and it might not be readily apparent, so I do see an argument to be made to add a helper option that does this for you. But I'd steer clear from doing it implicitly, possibly without user knowledge. Considering the amount of novice developers that would just simply omit setting this option I'd prefer it to stay off by default.

And when adding this option to readme I'd add a big note about possible security implications.

from docker-compose.

@AleF83 I had the same issue. The problem here is that when you specify the env param, child_process does not merge it with the current env variables, so you have to do it manually:

const options: dockerCompose.IDockerComposeBuildOptions = {
  cwd,
  env: {
    COMPOSE_DOCKER_CLI_BUILD: '1',
    DOCKER_BUILDKIT: '1',
    ...process.env,
  },
  log: true,
};

Doing so should make your script work.

Thanks!

But this looks very strange and not intuitive.

I think that the env option should receive only additional or altering variables.
The base should be or some defaults object or process.env.

The fact that the library uses child_process module is implementation details and the user shouldn't be aware of it.

from docker-compose.

But this looks very strange and not intuitive.

I disagree with this statement.
Parameter destructuring is a popular way of expanding variables in TypeScript (practically equal to calling Object.assign({}, process.env) in pure JS. And it's already part of ES2018 spec.

The fact that env option is exposed allows you to adjust exactly how docker-compose binary will be called. Usage of child_process is irrelevant - practically all libraries both in Node.js and other languages / frameworks known to me that launch subprocesses will not populate environment variables (if anything, for the security reason above).

The base should be or some defaults object or process.env.

Currently no defaults are passed because docker-compose works fine without any. Trying to guess what the programmer-user intent is leads only to confusion ("why this is default, not the other?" / "it conflicts with my default settings!").
Give us a compelling reason to provide such defaults.
Making Docker Buildkit usage default would mean you'd need to explicitly roll back to default docker-compose setting, while causing existing users to start using experimental builder without asking them first (BC break).

If you do not want to populate subprocess with your whole process.env object then you're perfectly fine with taking only specific properties. Nothing stops you, it's your code / use-case after all.

All-in-all, what's missing is documentation, not helper code.

from docker-compose.

I didn't mean object destructoring but using the process.env to build env options. I understand that it's default for child_process module but it's still weird.

In the bottom line:

COMPOSE_DOCKER_CLI_BUILD=1 DOCKER_BUILDKIT=1 docker-compose -f <file> up

works.

await dockerCompose.upAll({
      cwd: __dirname,
      env: {
        COMPOSE_DOCKER_CLI_BUILD: '1',
        DOCKER_BUILDKIT: '1',
      },
    });

doesn't work on the same machine.

And this is weird.

from docker-compose.

@AleF83 try getting the output from result.

export interface IDockerComposeResult {
  exitCode: number | null;
  out: string;
  err: string;
}

This result object should be available whether or not promise is resolved or rejected and should contain output from the binary. We could then see what the issue is.

You can also pass log: true as option to increase verbosity.

from docker-compose.

Addition of ...process.env makes it work.
Without I get this:

Error: spawn docker-compose ENOENT
    at Process.ChildProcess._handle.onexit (internal/child_process.js:268:19)
    at onErrorNT (internal/child_process.js:468:16)
    at processTicksAndRejections (internal/process/task_queues.js:84:21) {
  errno: -2,
  code: 'ENOENT',
  syscall: 'spawn docker-compose',
  path: 'docker-compose',
  spawnargs: [ 'build' ]
}

I printed process.env out but didn't see anything related to docker. Maybe PATH or something like this...

from docker-compose.

@AleF83 yeah, looks like it can't find docker-compose binary. @AlexZeitler how do you want to approach this? We could allow to specify precise docker-compose location or try to find it before spawning processes. Passing PATH into env isn't particularly bad in this case?

from docker-compose.

With PATH it works:

await dockerCompose.buildAll({
      cwd: __dirname,
      env: {
        COMPOSE_DOCKER_CLI_BUILD: '1',
        DOCKER_BUILDKIT: '1',
        PATH: process.env.PATH,
      },
      log: true,
    });

Maybe the PATH can be the default for env option. All the variables the user set explicitly in env will be added to this default.

from docker-compose.