-
Notifications
You must be signed in to change notification settings - Fork 42
Use timeout utility instead of double-sub-shell hell #380
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
Conversation
@SlouchyButton Any idea why |
60c8c81
to
fbc78b3
Compare
[test] |
fbc78b3
to
09eaab2
Compare
[test] |
09eaab2
to
52a1c32
Compare
[test] |
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.
Tested on Node.js container on all supported targets and versions. All tests seem to be working.
Code is cleaner and more readable.
No, I don't think it could be a problem. Only thing to keep in mind is that it is part of |
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. Let's spread it into containers and we will see the updates.
[test] |
It turned out that for incremental builds for Node.js, the test always waited 10m no matter that the build finished sooner. It's not clear why it happened, but when debugging, it seemed like some weired race-condition happened that let some sleeps hang. Anyway, it's not clear whether there is a reason to not use timeout utility that seems to serve exactly for this reason.
52a1c32
to
fc8bf0e
Compare
[test] |
Let's get merge it and distribute to s2i-nodejs-container as first. |
It turned out that for incremental builds for Node.js, the test always waited 10m no matter that the build finished sooner.
It's not clear why it happened, but when debugging, it seemed like some weired race-condition happened that let some sleeps hang.
Anyway, it's not clear whether there is a reason to not use timeout utility that seems to serve exactly for this reason.