Git Product home page Git Product logo

Comments (16)

AlexZeitler avatar AlexZeitler commented on July 29, 2024 1

@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.

sbalay avatar sbalay commented on July 29, 2024 1

Makes sense! I'll send the PR.

from docker-compose.

Steveb-p avatar Steveb-p commented on July 29, 2024 1

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.

sbalay avatar sbalay commented on July 29, 2024 1

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.

sbalay avatar sbalay commented on July 29, 2024

@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.

Steveb-p avatar Steveb-p commented on July 29, 2024

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.

sbalay avatar sbalay commented on July 29, 2024

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.

sbalay avatar sbalay commented on July 29, 2024

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.

Steveb-p avatar Steveb-p commented on July 29, 2024

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 avatar AleF83 commented on July 29, 2024

@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.

Steveb-p avatar Steveb-p commented on July 29, 2024

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.

AleF83 avatar AleF83 commented on July 29, 2024

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.

Steveb-p avatar Steveb-p commented on July 29, 2024

@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.

AleF83 avatar AleF83 commented on July 29, 2024

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.

Steveb-p avatar Steveb-p commented on July 29, 2024

@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.

AleF83 avatar AleF83 commented on July 29, 2024

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.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.