Skip to content

Commit ae893d9

Browse files
committed
add --skip-forward-ports flag to disable this
- per review comments, GH Codespaces and VS Code extension use their own implementation for `forwardPorts` and so would conflict with Docker publishing - so `--skip-forward-ports` allows for publishing of `forwardPorts` to be skipped by the CLI NOTE: passing `skipForwardPorts` around required drilling down through a lot of function arguments -- those could potentially be refactored, but that is out-of-scope for now
1 parent d0df3bb commit ae893d9

File tree

5 files changed

+17
-12
lines changed

5 files changed

+17
-12
lines changed

src/spec-node/configContainer.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,19 +23,19 @@ import { DockerCLIParameters } from '../spec-shutdown/dockerUtils';
2323
import { createDocuments } from '../spec-configuration/editableFiles';
2424

2525

26-
export async function resolve(params: DockerResolverParameters, configFile: URI | undefined, overrideConfigFile: URI | undefined, providedIdLabels: string[] | undefined, additionalFeatures: Record<string, string | boolean | Record<string, string | boolean>>): Promise<ResolverResult> {
26+
export async function resolve(params: DockerResolverParameters, configFile: URI | undefined, overrideConfigFile: URI | undefined, providedIdLabels: string[] | undefined, additionalFeatures: Record<string, string | boolean | Record<string, string | boolean>>, skipForwardPorts: boolean): Promise<ResolverResult> {
2727
if (configFile && !/\/\.?devcontainer\.json$/.test(configFile.path)) {
2828
throw new Error(`Filename must be devcontainer.json or .devcontainer.json (${uriToFsPath(configFile, params.common.cliHost.platform)}).`);
2929
}
3030
const parsedAuthority = params.parsedAuthority;
3131
if (!parsedAuthority || isDevContainerAuthority(parsedAuthority)) {
32-
return resolveWithLocalFolder(params, parsedAuthority, configFile, overrideConfigFile, providedIdLabels, additionalFeatures);
32+
return resolveWithLocalFolder(params, parsedAuthority, configFile, overrideConfigFile, providedIdLabels, additionalFeatures, skipForwardPorts);
3333
} else {
3434
throw new Error(`Unexpected authority: ${JSON.stringify(parsedAuthority)}`);
3535
}
3636
}
3737

38-
async function resolveWithLocalFolder(params: DockerResolverParameters, parsedAuthority: DevContainerAuthority | undefined, configFile: URI | undefined, overrideConfigFile: URI | undefined, providedIdLabels: string[] | undefined, additionalFeatures: Record<string, string | boolean | Record<string, string | boolean>>): Promise<ResolverResult> {
38+
async function resolveWithLocalFolder(params: DockerResolverParameters, parsedAuthority: DevContainerAuthority | undefined, configFile: URI | undefined, overrideConfigFile: URI | undefined, providedIdLabels: string[] | undefined, additionalFeatures: Record<string, string | boolean | Record<string, string | boolean>>, skipForwardPorts: boolean): Promise<ResolverResult> {
3939
const { common, workspaceMountConsistencyDefault } = params;
4040
const { cliHost, output } = common;
4141

@@ -67,7 +67,7 @@ async function resolveWithLocalFolder(params: DockerResolverParameters, parsedAu
6767

6868
let result: ResolverResult;
6969
if (isDockerFileConfig(config) || 'image' in config) {
70-
result = await openDockerfileDevContainer(params, configWithRaw as SubstitutedConfig<DevContainerFromDockerfileConfig | DevContainerFromImageConfig>, configs.workspaceConfig, idLabels, additionalFeatures);
70+
result = await openDockerfileDevContainer(params, configWithRaw as SubstitutedConfig<DevContainerFromDockerfileConfig | DevContainerFromImageConfig>, configs.workspaceConfig, idLabels, additionalFeatures, skipForwardPorts);
7171
} else if ('dockerComposeFile' in config) {
7272
if (!workspace) {
7373
throw new ContainerError({ description: `A Dev Container using Docker Compose requires a workspace folder.` });

src/spec-node/devContainers.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ export interface ProvisionOptions {
5858
buildxCacheTo: string | undefined;
5959
additionalFeatures?: Record<string, string | boolean | Record<string, string | boolean>>;
6060
skipFeatureAutoMapping: boolean;
61+
skipForwardPorts: boolean;
6162
skipPostAttach: boolean;
6263
containerSessionDataFolder?: string;
6364
skipPersistingCustomizationsFromFeatures: boolean;
@@ -81,7 +82,7 @@ export async function launch(options: ProvisionOptions, providedIdLabels: string
8182
const text = 'Resolving Remote';
8283
const start = output.start(text);
8384

84-
const result = await resolve(params, options.configFile, options.overrideConfigFile, providedIdLabels, options.additionalFeatures ?? {});
85+
const result = await resolve(params, options.configFile, options.overrideConfigFile, providedIdLabels, options.additionalFeatures ?? {}, options.skipForwardPorts);
8586
output.stop(text, start);
8687
const { dockerContainerId, composeProjectName } = result;
8788
return {

src/spec-node/devContainersSpecCLI.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ function provisionOptions(y: Argv) {
130130
'buildkit': { choices: ['auto' as 'auto', 'never' as 'never'], default: 'auto' as 'auto', description: 'Control whether BuildKit should be used' },
131131
'additional-features': { type: 'string', description: 'Additional features to apply to the dev container (JSON as per "features" section in devcontainer.json)' },
132132
'skip-feature-auto-mapping': { type: 'boolean', default: false, hidden: true, description: 'Temporary option for testing.' },
133+
'skip-forward-ports': { type: 'boolean', default: false, description: 'Do not publish forwardPorts.' },
133134
'skip-post-attach': { type: 'boolean', default: false, description: 'Do not run postAttachCommand.' },
134135
'dotfiles-repository': { type: 'string', description: 'URL of a dotfiles Git repository (e.g., https://github.com/owner/repository.git)' },
135136
'dotfiles-install-command': { type: 'string', description: 'The command to run after cloning the dotfiles repository. Defaults to run the first file of `install.sh`, `install`, `bootstrap.sh`, `bootstrap`, `setup.sh` and `setup` found in the dotfiles repository`s root folder.' },
@@ -204,6 +205,7 @@ async function provision({
204205
'buildkit': buildkit,
205206
'additional-features': additionalFeaturesJson,
206207
'skip-feature-auto-mapping': skipFeatureAutoMapping,
208+
'skip-forward-ports': skipForwardPorts,
207209
'skip-post-attach': skipPostAttach,
208210
'dotfiles-repository': dotfilesRepository,
209211
'dotfiles-install-command': dotfilesInstallCommand,
@@ -277,6 +279,7 @@ async function provision({
277279
buildxCacheTo: addCacheTo,
278280
additionalFeatures,
279281
skipFeatureAutoMapping,
282+
skipForwardPorts,
280283
skipPostAttach,
281284
containerSessionDataFolder,
282285
skipPersistingCustomizationsFromFeatures: false,
@@ -680,7 +683,7 @@ async function doBuild({
680683
if (envFile) {
681684
composeGlobalArgs.push('--env-file', envFile);
682685
}
683-
686+
684687
const composeConfig = await readDockerComposeConfig(buildParams, composeFiles, envFile);
685688
const projectName = await getProjectName(params, workspace, composeFiles, composeConfig);
686689
const services = Object.keys(composeConfig.services || {});

src/spec-node/ports.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ function normalizePorts(ports: number | string | (number | string)[] | undefined
66
return ports.map((port) => typeof port === 'number' ? `127.0.0.1:${port}:${port}`: port);
77
}
88

9-
export function getStaticPorts(config: DevContainerFromDockerfileConfig | DevContainerFromImageConfig): string[] {
10-
return normalizePorts(config.forwardPorts).concat(normalizePorts(config.appPort));
9+
export function getStaticPorts(config: DevContainerFromDockerfileConfig | DevContainerFromImageConfig, skipForwardPorts: boolean): string[] {
10+
const forwardPorts = skipForwardPorts ? undefined : config.forwardPorts;
11+
return normalizePorts(forwardPorts).concat(normalizePorts(config.appPort));
1112
}

src/spec-node/singleContainer.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import { getStaticPorts } from './ports';
1818
export const hostFolderLabel = 'devcontainer.local_folder'; // used to label containers created from a workspace/folder
1919
export const configFileLabel = 'devcontainer.config_file';
2020

21-
export async function openDockerfileDevContainer(params: DockerResolverParameters, configWithRaw: SubstitutedConfig<DevContainerFromDockerfileConfig | DevContainerFromImageConfig>, workspaceConfig: WorkspaceConfiguration, idLabels: string[], additionalFeatures: Record<string, string | boolean | Record<string, string | boolean>>): Promise<ResolverResult> {
21+
export async function openDockerfileDevContainer(params: DockerResolverParameters, configWithRaw: SubstitutedConfig<DevContainerFromDockerfileConfig | DevContainerFromImageConfig>, workspaceConfig: WorkspaceConfiguration, idLabels: string[], additionalFeatures: Record<string, string | boolean | Record<string, string | boolean>>, skipForwardPorts: boolean): Promise<ResolverResult> {
2222
const { common } = params;
2323
const { config } = configWithRaw;
2424
// let collapsedFeaturesConfig: () => Promise<CollapsedFeaturesConfig | undefined>;
@@ -52,7 +52,7 @@ export async function openDockerfileDevContainer(params: DockerResolverParameter
5252
// collapsedFeaturesConfig = async () => res.collapsedFeaturesConfig;
5353

5454
try {
55-
await spawnDevContainer(params, config, mergedConfig, updatedImageName, idLabels, workspaceConfig.workspaceMount, res.imageDetails, containerUser, res.labels || {});
55+
await spawnDevContainer(params, config, mergedConfig, updatedImageName, idLabels, workspaceConfig.workspaceMount, res.imageDetails, containerUser, res.labels || {}, skipForwardPorts);
5656
} finally {
5757
// In 'finally' because 'docker run' can fail after creating the container.
5858
// Trying to get it here, so we can offer 'Rebuild Container' as an action later.
@@ -345,11 +345,11 @@ export async function extraRunArgs(common: ResolverParameters, params: DockerRes
345345
return extraArguments;
346346
}
347347

348-
export async function spawnDevContainer(params: DockerResolverParameters, config: DevContainerFromDockerfileConfig | DevContainerFromImageConfig, mergedConfig: MergedDevContainerConfig, imageName: string, labels: string[], workspaceMount: string | undefined, imageDetails: (() => Promise<ImageDetails>) | undefined, containerUser: string | undefined, extraLabels: Record<string, string>) {
348+
export async function spawnDevContainer(params: DockerResolverParameters, config: DevContainerFromDockerfileConfig | DevContainerFromImageConfig, mergedConfig: MergedDevContainerConfig, imageName: string, labels: string[], workspaceMount: string | undefined, imageDetails: (() => Promise<ImageDetails>) | undefined, containerUser: string | undefined, extraLabels: Record<string, string>, skipForwardPorts: boolean) {
349349
const { common } = params;
350350
common.progress(ResolverProgress.StartingContainer);
351351

352-
const exposedPorts = getStaticPorts(config);
352+
const exposedPorts = getStaticPorts(config, skipForwardPorts);
353353
const exposed = exposedPorts.flatMap((port) => ['-p', port]);
354354

355355
const cwdMount = workspaceMount ? ['--mount', workspaceMount] : [];

0 commit comments

Comments
 (0)