Skip to content

Commit d2b4614

Browse files
EhabYEhabY
committed
Move RemoteCommand logging into buildSshOverrides
Pass the logger into buildSshOverrides and log there, where the enableRemoteCommand setting is actually checked. This avoids exposing isActiveRemoteCommand, which alone does not account for the setting.
1 parent f2b04e7 commit d2b4614

File tree

3 files changed

+43
-35
lines changed

3 files changed

+43
-35
lines changed

‎src/remote/remote.ts‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,7 @@ import {
5858
parseCoderSshOptions,
5959
parseSshConfig,
6060
} from "./sshConfig";
61-
import {
62-
applySettingOverrides,
63-
buildSshOverrides,
64-
isActiveRemoteCommand,
65-
} from "./sshOverrides";
61+
import { applySettingOverrides, buildSshOverrides } from "./sshOverrides";
6662
import { SshProcessMonitor } from "./sshProcess";
6763
import { computeSshProperties, sshSupportsSetEnv } from "./sshSupport";
6864
import { WorkspaceStateMachine } from "./workspaceStateMachine";
@@ -484,18 +480,14 @@ export class Remote {
484480
}
485481

486482
const remoteCommand = computedSshProperties.RemoteCommand;
487-
if (isActiveRemoteCommand(remoteCommand)) {
488-
this.logger.info(
489-
"RemoteCommand detected, skipping remotePlatform override",
490-
);
491-
}
492483

493484
this.logger.info("Modifying settings...");
494485
const overrides = buildSshOverrides(
495486
vscodeProposed.workspace.getConfiguration(),
496487
parts.sshHost,
497488
agent.operating_system,
498489
remoteCommand,
490+
this.logger,
499491
);
500492
if (overrides.length > 0) {
501493
const ok = await applySettingOverrides(

‎src/remote/sshOverrides.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ const AUTO_SETUP_DEFAULTS = {
6363
* Whether the given RemoteCommand value represents an active command
6464
* (i.e. present, non-empty, and not the SSH default "none").
6565
*/
66-
export function isActiveRemoteCommand(cmd: string | undefined): boolean {
66+
function isActiveRemoteCommand(cmd: string | undefined): boolean {
6767
return !!cmd && cmd.toLowerCase() !== "none";
6868
}
6969

@@ -76,6 +76,7 @@ export function buildSshOverrides(
7676
sshHost: string,
7777
agentOS: string,
7878
remoteCommand: string | undefined,
79+
logger: Logger,
7980
): SettingOverride[] {
8081
const overrides: SettingOverride[] = [];
8182

@@ -96,6 +97,7 @@ export function buildSshOverrides(
9697
{},
9798
);
9899
if (skipRemotePlatform) {
100+
logger.info("RemoteCommand detected, skipping remotePlatform override");
99101
// Remove any stale entry so it doesn't block RemoteCommand.
100102
if (sshHost in remotePlatforms) {
101103
const { [sshHost]: _removed, ...rest } = remotePlatforms;

‎test/unit/remote/sshOverrides.test.ts‎

Lines changed: 38 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import { beforeEach, describe, expect, it, vi } from "vitest";
55
import {
66
applySettingOverrides,
77
buildSshOverrides,
8-
isActiveRemoteCommand,
98
} from "@/remote/sshOverrides";
109

1110
import {
@@ -33,19 +32,8 @@ interface TimeoutCase {
3332
label: string;
3433
}
3534

36-
describe("isActiveRemoteCommand", () => {
37-
it.each(["exec bash -l", "exec /bin/zsh", "/usr/bin/tmux"])(
38-
"returns true for %j",
39-
(cmd) => expect(isActiveRemoteCommand(cmd)).toBe(true),
40-
);
41-
42-
it.each([undefined, "", "none", "None", "NONE"])(
43-
"returns false for %j",
44-
(cmd) => expect(isActiveRemoteCommand(cmd)).toBe(false),
45-
);
46-
});
47-
4835
describe("buildSshOverrides", () => {
36+
const buildLogger = createMockLogger();
4937
describe("remote platform", () => {
5038
it("adds host when missing or OS differs", () => {
5139
const config = new MockConfigurationProvider();
@@ -54,7 +42,13 @@ describe("buildSshOverrides", () => {
5442
config.set("remote.SSH.remotePlatform", { "other-host": "darwin" });
5543
expect(
5644
findOverride(
57-
buildSshOverrides(config, "new-host", "linux", undefined),
45+
buildSshOverrides(
46+
config,
47+
"new-host",
48+
"linux",
49+
undefined,
50+
buildLogger,
51+
),
5852
"remote.SSH.remotePlatform",
5953
),
6054
).toEqual({ "other-host": "darwin", "new-host": "linux" });
@@ -63,7 +57,7 @@ describe("buildSshOverrides", () => {
6357
config.set("remote.SSH.remotePlatform", { "my-host": "windows" });
6458
expect(
6559
findOverride(
66-
buildSshOverrides(config, "my-host", "linux", undefined),
60+
buildSshOverrides(config, "my-host", "linux", undefined, buildLogger),
6761
"remote.SSH.remotePlatform",
6862
),
6963
).toEqual({ "my-host": "linux" });
@@ -74,7 +68,7 @@ describe("buildSshOverrides", () => {
7468
config.set("remote.SSH.remotePlatform", { "my-host": "linux" });
7569
expect(
7670
findOverride(
77-
buildSshOverrides(config, "my-host", "linux", undefined),
71+
buildSshOverrides(config, "my-host", "linux", undefined, buildLogger),
7872
"remote.SSH.remotePlatform",
7973
),
8074
).toBeUndefined();
@@ -90,7 +84,13 @@ describe("buildSshOverrides", () => {
9084
});
9185
expect(
9286
findOverride(
93-
buildSshOverrides(config, "my-host", "linux", "exec bash -l"),
87+
buildSshOverrides(
88+
config,
89+
"my-host",
90+
"linux",
91+
"exec bash -l",
92+
buildLogger,
93+
),
9494
"remote.SSH.remotePlatform",
9595
),
9696
).toEqual({ "other-host": "darwin" });
@@ -102,7 +102,13 @@ describe("buildSshOverrides", () => {
102102
config.set("remote.SSH.remotePlatform", {});
103103
expect(
104104
findOverride(
105-
buildSshOverrides(config, "my-host", "linux", "exec bash -l"),
105+
buildSshOverrides(
106+
config,
107+
"my-host",
108+
"linux",
109+
"exec bash -l",
110+
buildLogger,
111+
),
106112
"remote.SSH.remotePlatform",
107113
),
108114
).toBeUndefined();
@@ -114,7 +120,13 @@ describe("buildSshOverrides", () => {
114120
config.set("remote.SSH.remotePlatform", {});
115121
expect(
116122
findOverride(
117-
buildSshOverrides(config, "my-host", "linux", "exec bash -l"),
123+
buildSshOverrides(
124+
config,
125+
"my-host",
126+
"linux",
127+
"exec bash -l",
128+
buildLogger,
129+
),
118130
"remote.SSH.remotePlatform",
119131
),
120132
).toEqual({ "my-host": "linux" });
@@ -128,7 +140,7 @@ describe("buildSshOverrides", () => {
128140
config.set("remote.SSH.remotePlatform", {});
129141
expect(
130142
findOverride(
131-
buildSshOverrides(config, "my-host", "linux", cmd),
143+
buildSshOverrides(config, "my-host", "linux", cmd, buildLogger),
132144
"remote.SSH.remotePlatform",
133145
),
134146
).toEqual({ "my-host": "linux" });
@@ -150,7 +162,7 @@ describe("buildSshOverrides", () => {
150162
}
151163
expect(
152164
findOverride(
153-
buildSshOverrides(config, "host", "linux", undefined),
165+
buildSshOverrides(config, "host", "linux", undefined, buildLogger),
154166
"remote.SSH.connectTimeout",
155167
),
156168
).toBe(1800);
@@ -164,7 +176,7 @@ describe("buildSshOverrides", () => {
164176
config.set("remote.SSH.connectTimeout", timeout);
165177
expect(
166178
findOverride(
167-
buildSshOverrides(config, "host", "linux", undefined),
179+
buildSshOverrides(config, "host", "linux", undefined, buildLogger),
168180
"remote.SSH.connectTimeout",
169181
),
170182
).toBeUndefined();
@@ -180,6 +192,7 @@ describe("buildSshOverrides", () => {
180192
"host",
181193
"linux",
182194
undefined,
195+
buildLogger,
183196
),
184197
"remote.SSH.reconnectionGraceTime",
185198
),
@@ -191,7 +204,7 @@ describe("buildSshOverrides", () => {
191204
config.set("remote.SSH.reconnectionGraceTime", 3600);
192205
expect(
193206
findOverride(
194-
buildSshOverrides(config, "host", "linux", undefined),
207+
buildSshOverrides(config, "host", "linux", undefined, buildLogger),
195208
"remote.SSH.reconnectionGraceTime",
196209
),
197210
).toBeUndefined();
@@ -207,6 +220,7 @@ describe("buildSshOverrides", () => {
207220
"host",
208221
"linux",
209222
undefined,
223+
buildLogger,
210224
);
211225
expect(findOverride(overrides, key)).toBe(expected);
212226
});
@@ -219,7 +233,7 @@ describe("buildSshOverrides", () => {
219233
config.set("remote.SSH.serverShutdownTimeout", 600);
220234
config.set("remote.SSH.maxReconnectionAttempts", 4);
221235
expect(
222-
buildSshOverrides(config, "my-host", "linux", undefined),
236+
buildSshOverrides(config, "my-host", "linux", undefined, buildLogger),
223237
).toHaveLength(0);
224238
});
225239
});

0 commit comments

Comments
 (0)