Skip to content

Commit

Permalink
Merge pull request #392 from devcontainers/samruddhikhandale/containe…
Browse files Browse the repository at this point in the history
…renv-bug

Bug fix: Merge metadata logic for containerEnv - devcontainer build
  • Loading branch information
samruddhikhandale authored Feb 3, 2023
2 parents 8e8c92c + dac6ee3 commit 77172a2
Show file tree
Hide file tree
Showing 6 changed files with 53 additions and 1 deletion.
2 changes: 2 additions & 0 deletions src/spec-configuration/containerFeaturesConfiguration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,8 @@ ARG _DEV_CONTAINERS_IMAGE_USER=root
USER $_DEV_CONTAINERS_IMAGE_USER
#{devcontainerMetadata}
#{containerEnvMetadata}
`;
}

Expand Down
3 changes: 2 additions & 1 deletion src/spec-node/containerFeatures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { readLocalFile } from '../spec-utils/pfs';
import { includeAllConfiguredFeatures } from '../spec-utils/product';
import { createFeaturesTempFolder, DockerResolverParameters, getCacheFolder, getFolderImageName, getEmptyContextFolder, SubstitutedConfig } from './utils';
import { isEarlierVersion, parseVersion } from '../spec-common/commonUtils';
import { getDevcontainerMetadata, getDevcontainerMetadataLabel, getImageBuildInfoFromImage, ImageBuildInfo, ImageMetadataEntry, imageMetadataLabel, MergedDevContainerConfig } from './imageMetadata';
import { getContainerEnvMetadata, getDevcontainerMetadata, getDevcontainerMetadataLabel, getImageBuildInfoFromImage, ImageBuildInfo, ImageMetadataEntry, imageMetadataLabel, MergedDevContainerConfig } from './imageMetadata';
import { supportsBuildContexts } from './dockerfileUtils';
import { ContainerError } from '../spec-common/errors';

Expand Down Expand Up @@ -293,6 +293,7 @@ async function getFeaturesBuildOptions(params: DockerResolverParameters, devCont
.replace('#{containerEnv}', generateContainerEnvs(featuresConfig))
.replace('#{copyFeatureBuildStages}', getCopyFeatureBuildStages(featuresConfig, buildStageScripts))
.replace('#{devcontainerMetadata}', getDevcontainerMetadataLabel(imageMetadata, common.experimentalImageMetadata))
.replace('#{containerEnvMetadata}', getContainerEnvMetadata(devContainerConfig.config.containerEnv))
;
const syntax = imageBuildInfo.dockerfile?.preamble.directives.syntax;
const dockerfilePrefixContent = `${useBuildKitBuildContexts && !(imageBuildInfo.dockerfile && supportsBuildContexts(imageBuildInfo.dockerfile)) ?
Expand Down
10 changes: 10 additions & 0 deletions src/spec-node/imageMetadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -443,3 +443,13 @@ function toLabelString(obj: object) {
return JSON.stringify(obj)
.replace(/(?=["\\$])/g, '\\');
}

export function getContainerEnvMetadata(containerEnv: Record<string, string> | undefined): string {
if (!containerEnv) {
return '';
}

const keys = Object.keys(containerEnv);
const concatenatedEnv = keys.map(k => `ENV ${k}=${containerEnv![k]}`).join('\n');
return concatenatedEnv;
}
15 changes: 15 additions & 0 deletions src/test/cli.build.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,5 +222,20 @@ describe('Dev Containers CLI', function () {
await shellExec(`docker buildx rm ${builderName}`);
}
});

it('should follow the correct merge logic for containerEnv', async () => {
const res = await shellExec(`${cli} build --workspace-folder ${__dirname}/configs/image-metadata-containerEnv --image-name "test-metadata"`);
const response = JSON.parse(res.stdout);
assert.equal(response.outcome, 'success');

const resRun = await shellExec(`docker run -it -d "test-metadata"`);
const containerId: string = resRun.stdout.split('\n')[0];
assert.ok(containerId, 'Container id not found.');

const result = await shellExec(`docker exec ${containerId} bash -c 'echo $JAVA_HOME'`);
assert.equal('/usr/lib/jvm/msopenjdk-current\n', result.stdout);

await shellExec(`docker rm -f ${containerId}`);
});
});
});
13 changes: 13 additions & 0 deletions src/test/cli.up.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,5 +165,18 @@ describe('Dev Containers CLI', function () {
});
});
});

it('should follow the correct merge logic for containerEnv', async () => {
const res = await shellExec(`${cli} up --workspace-folder ${__dirname}/configs/image-metadata-containerEnv`);
const response = JSON.parse(res.stdout);
assert.equal(response.outcome, 'success');
const containerId: string = response.containerId;
assert.ok(containerId, 'Container id not found.');

const result = await shellExec(`docker exec ${containerId} bash -c 'echo $JAVA_HOME'`);
assert.equal('/usr/lib/jvm/msopenjdk-current\n', result.stdout);

await shellExec(`docker rm -f ${containerId}`);
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"image": "mcr.microsoft.com/devcontainers/base:ubuntu",
"features": {
"ghcr.io/devcontainers/features/java:1": {
"version": "none"
}
},
"containerEnv": {
"JAVA_HOME": "/usr/lib/jvm/msopenjdk-current"
}
}

0 comments on commit 77172a2

Please sign in to comment.