Skip to content

Commit

Permalink
add --skip-forward-ports flag to disable this
Browse files Browse the repository at this point in the history
- 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
  • Loading branch information
agilgur5 committed Jan 16, 2025
1 parent d0df3bb commit ae893d9
Show file tree
Hide file tree
Showing 5 changed files with 17 additions and 12 deletions.
8 changes: 4 additions & 4 deletions src/spec-node/configContainer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,19 @@ import { DockerCLIParameters } from '../spec-shutdown/dockerUtils';
import { createDocuments } from '../spec-configuration/editableFiles';


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> {
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> {
if (configFile && !/\/\.?devcontainer\.json$/.test(configFile.path)) {
throw new Error(`Filename must be devcontainer.json or .devcontainer.json (${uriToFsPath(configFile, params.common.cliHost.platform)}).`);
}
const parsedAuthority = params.parsedAuthority;
if (!parsedAuthority || isDevContainerAuthority(parsedAuthority)) {
return resolveWithLocalFolder(params, parsedAuthority, configFile, overrideConfigFile, providedIdLabels, additionalFeatures);
return resolveWithLocalFolder(params, parsedAuthority, configFile, overrideConfigFile, providedIdLabels, additionalFeatures, skipForwardPorts);
} else {
throw new Error(`Unexpected authority: ${JSON.stringify(parsedAuthority)}`);
}
}

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> {
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> {
const { common, workspaceMountConsistencyDefault } = params;
const { cliHost, output } = common;

Expand Down Expand Up @@ -67,7 +67,7 @@ async function resolveWithLocalFolder(params: DockerResolverParameters, parsedAu

let result: ResolverResult;
if (isDockerFileConfig(config) || 'image' in config) {
result = await openDockerfileDevContainer(params, configWithRaw as SubstitutedConfig<DevContainerFromDockerfileConfig | DevContainerFromImageConfig>, configs.workspaceConfig, idLabels, additionalFeatures);
result = await openDockerfileDevContainer(params, configWithRaw as SubstitutedConfig<DevContainerFromDockerfileConfig | DevContainerFromImageConfig>, configs.workspaceConfig, idLabels, additionalFeatures, skipForwardPorts);
} else if ('dockerComposeFile' in config) {
if (!workspace) {
throw new ContainerError({ description: `A Dev Container using Docker Compose requires a workspace folder.` });
Expand Down
3 changes: 2 additions & 1 deletion src/spec-node/devContainers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export interface ProvisionOptions {
buildxCacheTo: string | undefined;
additionalFeatures?: Record<string, string | boolean | Record<string, string | boolean>>;
skipFeatureAutoMapping: boolean;
skipForwardPorts: boolean;
skipPostAttach: boolean;
containerSessionDataFolder?: string;
skipPersistingCustomizationsFromFeatures: boolean;
Expand All @@ -81,7 +82,7 @@ export async function launch(options: ProvisionOptions, providedIdLabels: string
const text = 'Resolving Remote';
const start = output.start(text);

const result = await resolve(params, options.configFile, options.overrideConfigFile, providedIdLabels, options.additionalFeatures ?? {});
const result = await resolve(params, options.configFile, options.overrideConfigFile, providedIdLabels, options.additionalFeatures ?? {}, options.skipForwardPorts);
output.stop(text, start);
const { dockerContainerId, composeProjectName } = result;
return {
Expand Down
5 changes: 4 additions & 1 deletion src/spec-node/devContainersSpecCLI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ function provisionOptions(y: Argv) {
'buildkit': { choices: ['auto' as 'auto', 'never' as 'never'], default: 'auto' as 'auto', description: 'Control whether BuildKit should be used' },
'additional-features': { type: 'string', description: 'Additional features to apply to the dev container (JSON as per "features" section in devcontainer.json)' },
'skip-feature-auto-mapping': { type: 'boolean', default: false, hidden: true, description: 'Temporary option for testing.' },
'skip-forward-ports': { type: 'boolean', default: false, description: 'Do not publish forwardPorts.' },
'skip-post-attach': { type: 'boolean', default: false, description: 'Do not run postAttachCommand.' },
'dotfiles-repository': { type: 'string', description: 'URL of a dotfiles Git repository (e.g., https://github.com/owner/repository.git)' },
'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.' },
Expand Down Expand Up @@ -204,6 +205,7 @@ async function provision({
'buildkit': buildkit,
'additional-features': additionalFeaturesJson,
'skip-feature-auto-mapping': skipFeatureAutoMapping,
'skip-forward-ports': skipForwardPorts,
'skip-post-attach': skipPostAttach,
'dotfiles-repository': dotfilesRepository,
'dotfiles-install-command': dotfilesInstallCommand,
Expand Down Expand Up @@ -277,6 +279,7 @@ async function provision({
buildxCacheTo: addCacheTo,
additionalFeatures,
skipFeatureAutoMapping,
skipForwardPorts,
skipPostAttach,
containerSessionDataFolder,
skipPersistingCustomizationsFromFeatures: false,
Expand Down Expand Up @@ -680,7 +683,7 @@ async function doBuild({
if (envFile) {
composeGlobalArgs.push('--env-file', envFile);
}

const composeConfig = await readDockerComposeConfig(buildParams, composeFiles, envFile);
const projectName = await getProjectName(params, workspace, composeFiles, composeConfig);
const services = Object.keys(composeConfig.services || {});
Expand Down
5 changes: 3 additions & 2 deletions src/spec-node/ports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ function normalizePorts(ports: number | string | (number | string)[] | undefined
return ports.map((port) => typeof port === 'number' ? `127.0.0.1:${port}:${port}`: port);
}

export function getStaticPorts(config: DevContainerFromDockerfileConfig | DevContainerFromImageConfig): string[] {
return normalizePorts(config.forwardPorts).concat(normalizePorts(config.appPort));
export function getStaticPorts(config: DevContainerFromDockerfileConfig | DevContainerFromImageConfig, skipForwardPorts: boolean): string[] {
const forwardPorts = skipForwardPorts ? undefined : config.forwardPorts;
return normalizePorts(forwardPorts).concat(normalizePorts(config.appPort));
}
8 changes: 4 additions & 4 deletions src/spec-node/singleContainer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { getStaticPorts } from './ports';
export const hostFolderLabel = 'devcontainer.local_folder'; // used to label containers created from a workspace/folder
export const configFileLabel = 'devcontainer.config_file';

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> {
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> {
const { common } = params;
const { config } = configWithRaw;
// let collapsedFeaturesConfig: () => Promise<CollapsedFeaturesConfig | undefined>;
Expand Down Expand Up @@ -52,7 +52,7 @@ export async function openDockerfileDevContainer(params: DockerResolverParameter
// collapsedFeaturesConfig = async () => res.collapsedFeaturesConfig;

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

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>) {
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) {
const { common } = params;
common.progress(ResolverProgress.StartingContainer);

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

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

0 comments on commit ae893d9

Please sign in to comment.