Skip to content

Commit 42f8279

Browse files
cursoragentcursoragent
committed
Fix duplicated env logic and eliminate redundant extractDetach call
- Bug 1: Replace duplicated COLUMNS/LINES injection logic in compose.js with buildEnvironment utility to ensure consistency with docker exec path - Bug 2: Refactor buildExecArgs to return both args and cleaned cmd, eliminating redundant extractDetach call - Update tests to handle buildExecArgs returning an object instead of just args array
1 parent 4b5d769 commit 42f8279

3 files changed

Lines changed: 35 additions & 40 deletions

File tree

‎lib/compose.js‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
const _ = require('lodash');
55
const describeContext = require('../utils/describe-context');
66
const extractDetach = require('../utils/extract-detach');
7+
const buildEnvironment = require('../utils/build-exec-environment');
78

89
// Helper object for flags
910
const composeFlags = {
@@ -143,17 +144,11 @@ exports.run = (compose, project, opts = {}) => {
143144
opts.noTTY = !(context.stdin.isTTY && context.stdout.isTTY);
144145
}
145146

146-
// Inject terminal-size and color hints so the compose exec path
147-
// gets the same treatment as the docker exec path. Caller-provided
148-
// environment vars always win via the spread order.
149-
const envDefaults = {};
150-
if (opts.noTTY) {
151-
envDefaults.COLUMNS = String(context.stdout.columns);
152-
envDefaults.LINES = String(context.stdout.rows);
153-
}
154-
if (context.landoColorLevel === 0) {
155-
envDefaults.NO_COLOR = '1';
156-
}
147+
// Use the shared buildEnvironment utility to inject terminal-size
148+
// and color hints, ensuring the compose exec path gets the same
149+
// treatment as the docker exec path. Caller-provided environment
150+
// vars always win via the spread order.
151+
const envDefaults = buildEnvironment(context);
157152
opts.environment = {...envDefaults, ...(opts.environment || {})};
158153

159154
return buildShell('exec', project, compose, opts);

‎test/tty-allocation.spec.js‎

Lines changed: 23 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -46,52 +46,52 @@ describe('TTY allocation', () => {
4646
describe('docker exec (utils/build-docker-exec.js)', () => {
4747
it('should include --tty when both stdin and stdout are TTYs', () => {
4848
const ctx = makeContext({stdin: {isTTY: true}, stdout: {isTTY: true}});
49-
const args = buildExecArgs('docker', makeDatum(), ctx);
49+
const {args} = buildExecArgs('docker', makeDatum(), ctx);
5050
expect(args).to.include('--tty');
5151
});
5252

5353
it('should not include --tty when stdout is not a TTY (output redirected)', () => {
5454
const ctx = makeContext({stdin: {isTTY: true}, stdout: {isTTY: false}});
55-
const args = buildExecArgs('docker', makeDatum(), ctx);
55+
const {args} = buildExecArgs('docker', makeDatum(), ctx);
5656
expect(args).to.not.include('--tty');
5757
});
5858

5959
it('should not include --tty when stdin is not a TTY', () => {
6060
const ctx = makeContext({stdin: {isTTY: false}, stdout: {isTTY: true}});
61-
const args = buildExecArgs('docker', makeDatum(), ctx);
61+
const {args} = buildExecArgs('docker', makeDatum(), ctx);
6262
expect(args).to.not.include('--tty');
6363
});
6464

6565
it('should not include --tty when neither stdin nor stdout is a TTY', () => {
6666
const ctx = makeContext({stdin: {isTTY: false}, stdout: {isTTY: false}});
67-
const args = buildExecArgs('docker', makeDatum(), ctx);
67+
const {args} = buildExecArgs('docker', makeDatum(), ctx);
6868
expect(args).to.not.include('--tty');
6969
});
7070
});
7171

7272
describe('interactive mode', () => {
7373
it('should include --interactive in node mode', () => {
7474
const ctx = makeContext({isNodeMode: true});
75-
const args = buildExecArgs('docker', makeDatum(), ctx);
75+
const {args} = buildExecArgs('docker', makeDatum(), ctx);
7676
expect(args).to.include('--interactive');
7777
});
7878

7979
it('should not include --interactive outside node mode', () => {
8080
const ctx = makeContext({isNodeMode: false});
81-
const args = buildExecArgs('docker', makeDatum(), ctx);
81+
const {args} = buildExecArgs('docker', makeDatum(), ctx);
8282
expect(args).to.not.include('--interactive');
8383
});
8484

8585
it('should not include --interactive when stdin is closed', () => {
8686
const ctx = makeContext({isNodeMode: true, stdin: {isTTY: true, isClosed: true}});
87-
const args = buildExecArgs('docker', makeDatum(), ctx);
87+
const {args} = buildExecArgs('docker', makeDatum(), ctx);
8888
expect(args).to.not.include('--interactive');
8989
});
9090

9191
it('should not include --interactive when detaching', () => {
9292
const ctx = makeContext({isNodeMode: true});
9393
const datum = makeDatum({cmd: ['sleep', '100', '&']});
94-
const args = buildExecArgs('docker', datum, ctx);
94+
const {args} = buildExecArgs('docker', datum, ctx);
9595
expect(args).to.include('--detach');
9696
expect(args).to.not.include('--interactive');
9797
});
@@ -101,22 +101,22 @@ describe('TTY allocation', () => {
101101
it('should detect trailing & and add --detach', () => {
102102
const ctx = makeContext();
103103
const datum = makeDatum({cmd: ['sleep', '100', '&']});
104-
const args = buildExecArgs('docker', datum, ctx);
104+
const {args} = buildExecArgs('docker', datum, ctx);
105105
expect(args).to.include('--detach');
106106
expect(args).to.not.include('&');
107107
});
108108

109109
it('should detect appended & in shell wrappers and add --detach', () => {
110110
const ctx = makeContext();
111111
const datum = makeDatum({cmd: ['/bin/sh', '-c', 'sleep 100&']});
112-
const args = buildExecArgs('docker', datum, ctx);
112+
const {args} = buildExecArgs('docker', datum, ctx);
113113
expect(args).to.include('--detach');
114114
});
115115

116116
it('should not include --tty when detaching', () => {
117117
const ctx = makeContext({stdin: {isTTY: true}, stdout: {isTTY: true}});
118118
const datum = makeDatum({cmd: ['sleep', '100', '&']});
119-
const args = buildExecArgs('docker', datum, ctx);
119+
const {args} = buildExecArgs('docker', datum, ctx);
120120
expect(args).to.include('--detach');
121121
expect(args).to.not.include('--tty');
122122
});
@@ -126,7 +126,7 @@ describe('TTY allocation', () => {
126126
it('should include workdir when set', () => {
127127
const ctx = makeContext();
128128
const datum = makeDatum({opts: {user: 'root', environment: {}, workdir: '/app'}});
129-
const args = buildExecArgs('docker', datum, ctx);
129+
const {args} = buildExecArgs('docker', datum, ctx);
130130
const wdIdx = args.indexOf('--workdir');
131131
expect(wdIdx).to.be.greaterThan(-1);
132132
expect(args[wdIdx + 1]).to.equal('/app');
@@ -135,7 +135,7 @@ describe('TTY allocation', () => {
135135
it('should include user', () => {
136136
const ctx = makeContext();
137137
const datum = makeDatum({opts: {user: 'root', environment: {}}});
138-
const args = buildExecArgs('docker', datum, ctx);
138+
const {args} = buildExecArgs('docker', datum, ctx);
139139
const uIdx = args.indexOf('--user');
140140
expect(uIdx).to.be.greaterThan(-1);
141141
expect(args[uIdx + 1]).to.equal('root');
@@ -144,15 +144,15 @@ describe('TTY allocation', () => {
144144
it('should include environment variables', () => {
145145
const ctx = makeContext();
146146
const datum = makeDatum({opts: {user: 'root', environment: {FOO: 'bar'}}});
147-
const args = buildExecArgs('docker', datum, ctx);
147+
const {args} = buildExecArgs('docker', datum, ctx);
148148
expect(args).to.include('--env');
149149
expect(args).to.include('FOO=bar');
150150
});
151151

152152
it('should place container id before the command', () => {
153153
const ctx = makeContext();
154154
const datum = makeDatum();
155-
const args = buildExecArgs('docker', datum, ctx);
155+
const {args} = buildExecArgs('docker', datum, ctx);
156156
const idIdx = args.indexOf('test_container');
157157
const cmdIdx = args.indexOf('echo');
158158
expect(idIdx).to.be.greaterThan(-1);
@@ -161,7 +161,7 @@ describe('TTY allocation', () => {
161161

162162
it('should use the specified docker binary', () => {
163163
const ctx = makeContext();
164-
const args = buildExecArgs('/usr/local/bin/docker', makeDatum(), ctx);
164+
const {args} = buildExecArgs('/usr/local/bin/docker', makeDatum(), ctx);
165165
expect(args[0]).to.equal('/usr/local/bin/docker');
166166
expect(args[1]).to.equal('exec');
167167
});
@@ -172,21 +172,18 @@ describe('TTY allocation', () => {
172172
const ctx = makeContext();
173173
const datum = makeDatum({cmd: ['sleep', '100', '&']});
174174
// Simulate what the exported module function does
175-
buildExecArgs('docker', datum, ctx);
175+
const {cmd} = buildExecArgs('docker', datum, ctx);
176176
// The internal buildExecArgs does NOT mutate, but the exported
177-
// wrapper writes back. Test the extractDetach write-back that
178-
// the outer function performs.
179-
const extractDetach = require('../utils/extract-detach');
180-
datum.cmd = extractDetach(datum.cmd).cmd;
181-
expect(datum.cmd).to.eql(['sleep', '100']);
182-
expect(datum.cmd).to.not.include('&');
177+
// wrapper writes back. Test that the returned cmd is cleaned.
178+
expect(cmd).to.eql(['sleep', '100']);
179+
expect(cmd).to.not.include('&');
183180
});
184181

185182
it('should write cleaned cmd for shell wrapper detach', () => {
183+
const ctx = makeContext();
186184
const datum = makeDatum({cmd: ['/bin/sh', '-c', 'sleep 100&']});
187-
const extractDetach = require('../utils/extract-detach');
188-
datum.cmd = extractDetach(datum.cmd).cmd;
189-
expect(datum.cmd).to.eql(['/bin/sh', '-c', 'sleep 100']);
185+
const {cmd} = buildExecArgs('docker', datum, ctx);
186+
expect(cmd).to.eql(['/bin/sh', '-c', 'sleep 100']);
190187
});
191188
});
192189

‎utils/build-docker-exec.js‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,9 @@ const buildEnvironment = require('./build-exec-environment');
1111
* environment propagation — reads from the context object rather than
1212
* from process globals directly. This makes every decision testable
1313
* with plain objects.
14+
*
15+
* Returns both the args array and the cleaned command for the caller
16+
* to use without re-parsing.
1417
*/
1518
const buildExecArgs = (docker, datum, context) => {
1619
const args = [docker, 'exec'];
@@ -46,19 +49,19 @@ const buildExecArgs = (docker, datum, context) => {
4649
args.push(datum.id);
4750
args.push(...cmd);
4851

49-
return args;
52+
return {args, cmd};
5053
};
5154

5255
module.exports = (injected, stdio, datum = {}) => {
5356
const dockerBin = injected.config.dockerBin || injected._config.dockerBin;
5457
const context = describeContext();
55-
const args = buildExecArgs(dockerBin, datum, context);
58+
const {args, cmd} = buildExecArgs(dockerBin, datum, context);
5659

5760
// Write the cleaned command back to datum so callers that reuse the
5861
// same object (e.g. build-tooling-task.js compose fallback) see it
5962
// without the trailing '&'. This preserves the mutation contract
6063
// the old getExecOpts() relied on.
61-
datum.cmd = extractDetach(datum.cmd).cmd;
64+
datum.cmd = cmd;
6265

6366
return injected.shell.sh(args, {mode: 'attach', cstdio: stdio});
6467
};

0 commit comments

Comments
 (0)