-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix running under docker #5135
Fix running under docker #5135
Conversation
@@ -351,6 +352,24 @@ export class JupyterExecutionBase implements IJupyterExecution { | |||
extraArgs.push('--debug'); | |||
} | |||
|
|||
// Check for a docker situation. |
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.
I feel this file jupyterExecution.ts
is just too large, including something as simple as this if
may simple and just a few lines, however the file just grows and now does a tonne of things..
extraArgs.push('127.0.0.1'); | ||
|
||
// Now see if we need --allow-root. | ||
const idResults = execSync('id', {encoding: 'utf-8'}); |
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.
I don't think we've got unit tests for this, if we're using execSync
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.
Nope no unit tests for this. Functional tests will attempt the lines before it though
@@ -351,6 +352,24 @@ export class JupyterExecutionBase implements IJupyterExecution { | |||
extraArgs.push('--debug'); | |||
} | |||
|
|||
// Check for a docker situation. | |||
try { | |||
const cgroup = await this.fileSystem.readFile('/proc/self/cgroup'); |
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.
More curiosity than a problem, but I would assume that this line will throw if the file can't be read. Basically we're using readFile to test for existence before reading the file. Is this a common way to do it? I would think you'd do an fs.exists and then read it if it exists.
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.
That seems like a good idea to me. I'll do that
For #5047
package-lock.json
has been regenerated by runningnpm install
(if dependencies have changed)