Git Product home page Git Product logo

Comments (11)

deser avatar deser commented on July 29, 2024 1

Thanks, seems it is

from docker-compose.

AlexZeitler avatar AlexZeitler commented on July 29, 2024 1

Great, I'll close this then...

from docker-compose.

AlexZeitler avatar AlexZeitler commented on July 29, 2024

@deser I created a rough fix for it in #75 but currently it breaks our tests due to an odd behaviour in docker-composes usage of stderr and stdout, see docker/compose#5590 for details.

@Steveb-p What are your thoughts on this?

from docker-compose.

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

@AlexZeitler looks good to me. It's exactly the kind of behavior that exit code reliance should fix.

EDIT: I'm looking confused at the CI errors :)

EDIT2: It seems that exit event is emitted before last data event? I'm not sure, gonna take a look at it running on my local machine.

from docker-compose.

AlexZeitler avatar AlexZeitler commented on July 29, 2024

@Steveb-p Sorry haven't got an update notification for your edit updates 😞

Looking at the failing test ensure exit code is returned correctly and https://github.com/PDMLab/docker-compose/blob/master/index.js#L69, I guess there's a conceptual issue between the result + exitCode and Promises rejecting.

from docker-compose.

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

@AlexZeitler Actually, I've checked this again after your comment. Apparently, there is nothing wrong with how EventEmitter for child processes behaves in relation to promises. It's just that now tests actually do reject, which causes awaits in tests to throw errors (since that's how async/await is supposed to work.

Previously they would just always resolve.

I'm in the process of reworking tests to take it into account. Also, I'm changing the reject() argument to result object instead of only exit code to allow working with stdout/stderr outputs if needed.

from docker-compose.

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

@AlexZeitler I'm working on it and migrating it to Jest, but kinda got stuck on some Docker issues for building. And apparently some building tests check wrong services - which I've noticed only after I've started migrating.

If you're gonna look on it further tomorrow (since it's 1:00 here and I have to go to work early in the morning and not be a worthless zombie), you'd probably want to change in your PR at the very least:
index.js (execCompose function)

  childProc.on('exit', exitCode => {
    result.exitCode = exitCode;
    if (exitCode === 0) {
      resolve(result);
    } else {
      reject(result);
    }
  });

So it rejects with stderr and output still available and without wrapping it with Error object.
Original in your PR for your reference:

  childProc.on('exit', exitCode => {
    result.exitCode = exitCode;
    if (exitCode !== 0) {
      return reject(new Error(result.err));
    }

    return resolve(result);
  });

In test/index.js, where you have:

assert.equal(1, (await compose.logs('non_existent_service', { cwd: path.join(__dirname) })).exitCode);

it should be replaced with something similar to:

  try {
    await compose.logs('non_existent_service', { cwd: path.join(__dirname) });
    assert.true(false, 'Execution should not reach this point');
  } catch (result) {
    assert.equal(result.exitCode, 1);
  }
  await compose.down({ cwd: path.join(__dirname), log: true });
  assert.end();

(also, notice missing compose.down call, which caused issues further down the line with orphan containers).

from docker-compose.

AlexZeitler avatar AlexZeitler commented on July 29, 2024

@Steveb-p That's what I noticed as well and tried and also tried to fix it.
Does your refactoring towards jest include the upAll fixes required for this issue or would you rely on my fix (which still has broken tests)? If you're refactoring towards jest I guess it doesn't make much sense for me to fix the tape based tests for now. But I'm not sure we should release my fix then until tests are stable again.

from docker-compose.

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

My jest based test crashed when building images due to docker errors (due to those tests running in parallel, which caused docker to conflict out operations). Otherwise I don't recall any other important issues.

I'll publish the PR in the evening when I have access to the laptop with relevant branch on.

EDIT: I'm pretty sure I based that branch on yours.

from docker-compose.

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

#77 should properly solve this case.

0.19 version of this package should now reject in this case.

from docker-compose.

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

@AlexZeitler I believe it should be closed, unless it still doesn't work in 0.19/0.20.

@deser try if it works for you, if possible.

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.