Skip to content

twilio cli inside a docker container - issues with npm pkg window-size #82

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

Closed
hcheg opened this issue Sep 28, 2019 · 3 comments · Fixed by #83
Closed

twilio cli inside a docker container - issues with npm pkg window-size #82

hcheg opened this issue Sep 28, 2019 · 3 comments · Fixed by #83
Labels
bug Something isn't working good first issue Good for newcomers hacktoberfest Hacktoberfest related contributions

Comments

@hcheg
Copy link

hcheg commented Sep 28, 2019

Hello!

I'm running the Twilio-cli inside a docker container, running "twilio server less:start" works perfectly if I run it manually from inside the running container but it fails when run using the docker CMD in the Dockerfile or from the entrypoint file. I didn't dig too much but it looks like the window-size npm pkg doesn't like to run without an actual console.

Here's the error I get:

twilio    | > twilio serverless:start
twilio    |
twilio    | TypeError: Cannot read property 'width' of undefined
twilio    |     at wrapText (~/.twilio-cli/node_modules/twilio-run/dist/printers/utils.js:32:76)
twilio    |     at importantMessage (~/.twilio-cli/node_modules/twilio-run/dist/printers/utils.js:35:13)
twilio    |     at Object.warningMessage (~/.twilio-cli/node_modules/twilio-run/dist/printers/utils.js:41:12)
twilio    |     at Logger.warn (~/.twilio-cli/node_modules/twilio-run/dist/utils/logger.js:49:23)
twilio    |     at printVersionWarning (~/.twilio-cli/node_modules/twilio-run/dist/checks/nodejs-version.js:16:21)
twilio    |     at Object.checkNodejsVersion [as default] (~/.twilio-cli/node_modules/twilio-run/dist/checks/nodejs-version.js:22:9)
twilio    |     at ~/.twilio-cli/node_modules/twilio-run/dist/commands/start.js:27:33
twilio    |     at Generator.next (<anonymous>)
twilio    |     at ~/.twilio-cli/node_modules/twilio-run/dist/commands/start.js:7:71
twilio    |     at new Promise (<anonymous>)
twilio    | npm ERR! code ELIFECYCLE
twilio    | npm ERR! errno 1
twilio    | npm ERR! [email protected] serverless-start: `twilio serverless:start`
~/.twilio-cli/node_modules/twilio-run/dist/printers/utils.js:32:76
---

const window_size_1 = __importDefault(require("window-size"));
..

###### I would perform a quick check on "window_size_1" and maybe use a default value ? ######
const wrapText = (text) => wrap_ansi_1.default(text, window_size_1.default.width - 5, { trim: false });
function importantMessage(label, color, title, body) {
    label = chalk_1.default.keyword(color)(label);
    title = wrapText(chalk_1.default.bold.underline(`${label} ${chalk_1.default.bold(title)}`));
    body = wrapText(body);
    return '\n' + borderLeft(`${title}\n\n${chalk_1.default.dim(body)}`, color) + '\n';
}
@hcheg
Copy link
Author

hcheg commented Sep 28, 2019

I found a workaround for now, I set the COLUMNS and ROWS environment variables and the problem is gone. "window-size" uses those internally.

So in the entrypoint, I used:

export COLUMNS=80
export ROWS=300

right before calling "twilio serverless:start".

I quickly tried setting the variables in the .env file instead but that didn't work though in theory it should've.

@dkundel
Copy link
Contributor

dkundel commented Oct 1, 2019

Oh thanks for catching this! The reason why the .env file is not being picked up is that the .env file is being loaded async and therefore won't be available when needed.

The fix in my opinion should be the following.

  1. Update the printers/utils.ts file to export a window size and have a fallback

So instead of the following: https://github.com/twilio-labs/twilio-run/blob/7bd4f7c6689c52fa36484e31aef716d3584ef0cb/src/printers/utils.ts#L4-L7

The code should be something like:

import size from 'window-size';

export const windowSize = size.get() || { width: 80, height: 300 }
  1. Update printers/utils.ts to use windowSize instead of size at https://github.com/twilio-labs/twilio-run/blob/7bd4f7c6689c52fa36484e31aef716d3584ef0cb/src/printers/utils.ts#L34

  2. printers/start.ts and printers/list.ts should instead import from utils.ts:

import { windowSize } from './utils';

@dkundel dkundel assigned dkundel and unassigned dkundel Oct 1, 2019
@dkundel dkundel added bug Something isn't working good first issue Good for newcomers hacktoberfest Hacktoberfest related contributions labels Oct 1, 2019
dkundel pushed a commit that referenced this issue Oct 3, 2019
use fixed windowSize when not available (in case of Docker CMD etc)

fix #82
@dkundel
Copy link
Contributor

dkundel commented Oct 3, 2019

Thanks for raising this @hcheg and thank you @saadismail for fixing it! The fix has been released in v1.1.1 of npm.im/@twilio-labs/plugin-serverless and v2.1.1 of npm.im/twilio-run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers hacktoberfest Hacktoberfest related contributions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants