-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Catch and print docker build errors #1499
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM.
Maybe add a comment to remove that once we upgrade to node16? #1457
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A [Uint8Array]
output will be unreadable though?
The original task asks to be able to see the output from underlying docker image builds. The reason we don't see that currently AFAIK is because a throw occurs before the printing to the terminal of the output of stdout
and stderr
- so you could move the throw
a few lines and then you will get the stderr/stdout output?
This is what the output with this PR would look like (in terms of the handling of stderr
/stdout
:
> child_process.spawnSync('bash', [`-c "echo testing; exit 1"`])
{
status: 2,
signal: null,
output: [
null,
<Buffer >,
<Buffer 62 61 73 68 3a 20 2d 20 3a 20 69 6e 76 61 6c 69 64 20 6f 70 74 69 6f 6e 0a 55 73 61 67 65 3a 09 62 61 73 68 20 5b 47 4e 55 20 6c 6f 6e 67 20 6f 70 74 ... 384 more bytes>
],
pid: 65063,
stdout: <Buffer >,
stderr: <Buffer 62 61 73 68 3a 20 2d 20 3a 20 69 6e 76 61 6c 69 64 20 6f 70 74 69 6f 6e 0a 55 73 61 67 65 3a 09 62 61 73 68 20 5b 47 4e 55 20 6c 6f 6e 67 20 6f 70 74 ... 384 more bytes>
}
And separately, 👍 @ Node 16
Yes good point. Erroring is only partially what we want; this message isn't helpful.
I looked and your comment in 1493 refers to airnode/docker/scripts/utils.ts Lines 5 to 8 in 520e07d
I'll have a look at taking what we need from the |
Updated! Now no need for Running command: 'exit 1; docker build --no-cache --build-arg npmRegistryUrl=http://172.17.0.7:4873 --build-arg npmTag=local --tag api3/airnode-admin-dev:local --file packages/airnode-admin/docker/Dockerfile packa
ges/airnode-admin/docker'
(node:1082) UnhandledPromiseRejectionWarning: Error:
Command failed with non-zero status code: 1
Stderr: .
Stdout: .
at runCommand (/build/docker/scripts/utils.ts:12:11)
at buildContainers (/build/docker/scripts/prepare-containers.ts:57:13)
at /build/docker/scripts/prepare-containers.ts:71:3
at Generator.next (<anonymous>)
at fulfilled (/build/docker/scripts/prepare-containers.ts:11:58)
at processTicksAndRejections (internal/process/task_queues.js:95:5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated! Now no need for
NODE_OPTIONS
. In the case of an error, the output is now (stdout
andstderr
are empty in this trivial example):
Looks good :), thanks for fixing.
Closes #1493.
Using
throw
withinNODE_OPTIONS='--unhandled-rejections=throw'
as this is the default for node v15.0.0 and above.I tested this by inserting
exit 1
before the airnode-admindocker build
command. Without this PR's change, the build "succeeds" despite the error, which reproduces the previous behavior:With this PR's change, the process exits as desired, and there are
stdout
andstderr
outputs, which I think suffices for our purposes (rather than needing everything streamed).