Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(integ-runner): ignoring asset changes doesn't work with new style assets #21638

Merged
merged 3 commits into from
Aug 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

This file was deleted.

51 changes: 39 additions & 12 deletions packages/@aws-cdk/integ-runner/lib/runner/private/cloud-assembly.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,49 +90,76 @@ export class AssemblyManifestReader {
}

/**
* For a given stackId return a list of assets that belong to the stack
* Return a list of assets for a given stack
*/
public getAssetsForStack(stackId: string): string[] {
public getAssetIdsForStack(stackId: string): string[] {
const assets: string[] = [];
for (const artifact of Object.values(this.manifest.artifacts ?? {})) {
if (artifact.type === ArtifactType.ASSET_MANIFEST && (artifact.properties as AssetManifestProperties)?.file === `${stackId}.assets.json`) {
assets.push(...this.assetsFromAssetManifest(artifact));
assets.push(...this.assetsFromAssetManifest(artifact).map(asset => asset.id.assetId));
} else if (artifact.type === ArtifactType.AWS_CLOUDFORMATION_STACK) {
assets.push(...this.assetsFromAssemblyManifest(artifact));
assets.push(...this.assetsFromAssemblyManifest(artifact).map(asset => asset.id));
}
}
return assets;
}

private assetsFromAssemblyManifest(artifact: ArtifactManifest): string[] {
/**
* For a given stackId return a list of assets that belong to the stack
*/
public getAssetLocationsForStack(stackId: string): string[] {
const assets: string[] = [];
for (const artifact of Object.values(this.manifest.artifacts ?? {})) {
if (artifact.type === ArtifactType.ASSET_MANIFEST && (artifact.properties as AssetManifestProperties)?.file === `${stackId}.assets.json`) {
assets.push(...this.assetsFromAssetManifest(artifact).map(asset => {
if (asset.type === 'file') {
return asset.source.path!;
} else {
return asset.source.directory!;
}
}));
} else if (artifact.type === ArtifactType.AWS_CLOUDFORMATION_STACK) {
assets.push(...this.assetsFromAssemblyManifest(artifact).map(asset => asset.path));
}
}
return assets;
}

/**
* Get a list of assets from the assembly manifest
*/
private assetsFromAssemblyManifest(artifact: ArtifactManifest): (ContainerImageAssetMetadataEntry | FileAssetMetadataEntry)[] {
const assets: (ContainerImageAssetMetadataEntry | FileAssetMetadataEntry)[] = [];
for (const metadata of Object.values(artifact.metadata ?? {})) {
metadata.forEach(data => {
if (data.type === ArtifactMetadataEntryType.ASSET) {
const assetPath = (data.data as ContainerImageAssetMetadataEntry | FileAssetMetadataEntry).path;
if (assetPath.startsWith('asset.')) {
assets.push(assetPath);
const asset = (data.data as ContainerImageAssetMetadataEntry | FileAssetMetadataEntry);
if (asset.path.startsWith('asset.')) {
assets.push(asset);
}
}
});
}
return assets;
}

private assetsFromAssetManifest(artifact: ArtifactManifest): string[] {
const assets: string[] = [];
/**
* Get a list of assets from the asset manifest
*/
private assetsFromAssetManifest(artifact: ArtifactManifest): (FileManifestEntry | DockerImageManifestEntry)[] {
const assets: (FileManifestEntry | DockerImageManifestEntry)[] = [];
const fileName = (artifact.properties as AssetManifestProperties).file;
const assetManifest = AssetManifest.fromFile(path.join(this.directory, fileName));
assetManifest.entries.forEach(entry => {
if (entry.type === 'file') {
const source = (entry as FileManifestEntry).source;
if (source.path && source.path.startsWith('asset.')) {
assets.push((entry as FileManifestEntry).source.path!);
assets.push(entry as FileManifestEntry);
}
} else if (entry.type === 'docker-image') {
const source = (entry as DockerImageManifestEntry).source;
if (source.directory && source.directory.startsWith('asset.')) {
assets.push((entry as DockerImageManifestEntry).source.directory!);
assets.push(entry as DockerImageManifestEntry);
}
}
});
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/integ-runner/lib/runner/runner-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ export abstract class IntegRunner {
const stacks = this.actualTestSuite.getStacksWithoutUpdateWorkflow() ?? [];
const manifest = AssemblyManifestReader.fromPath(this.snapshotDir);
const assets = flatten(stacks.map(stack => {
return manifest.getAssetsForStack(stack) ?? [];
return manifest.getAssetLocationsForStack(stack) ?? [];
}));

assets.forEach(asset => {
Expand Down
86 changes: 83 additions & 3 deletions packages/@aws-cdk/integ-runner/lib/runner/snapshot-test-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { Writable, WritableOptions } from 'stream';
import { StringDecoder } from 'string_decoder';
import { diffTemplate, formatDifferences, ResourceDifference, ResourceImpact } from '@aws-cdk/cloudformation-diff';
import { Diagnostic, DiagnosticReason, DestructiveChange, SnapshotVerificationOptions } from '../workers/common';
import { canonicalizeTemplate } from './private/canonicalize-assets';
import { AssemblyManifestReader } from './private/cloud-assembly';
import { IntegRunnerOptions, IntegRunner, DEFAULT_SYNTH_OPTIONS } from './runner-base';

Expand Down Expand Up @@ -166,8 +165,8 @@ export class IntegSnapshotRunner extends IntegRunner {
// asset hashes from the templates so they are not part of the diff
// comparison
if (!this.actualTestSuite.getOptionsForStack(templateId)?.diffAssets) {
actualTemplate = canonicalizeTemplate(actualTemplate);
expectedTemplate = canonicalizeTemplate(expectedTemplate);
actualTemplate = this.canonicalizeTemplate(actualTemplate, templateId, this.cdkOutDir);
expectedTemplate = this.canonicalizeTemplate(expectedTemplate, templateId, this.snapshotDir);
}
const templateDiff = diffTemplate(expectedTemplate, actualTemplate);
if (!templateDiff.isEmpty) {
Expand Down Expand Up @@ -225,6 +224,87 @@ export class IntegSnapshotRunner extends IntegRunner {

return stacks;
}

/**
* Reduce template to a normal form where asset references have been normalized
*
* This makes it possible to compare templates if all that's different between
* them is the hashes of the asset values.
*/
private canonicalizeTemplate(template: any, stackName: string, manifestDir: string): any {
const assetsSeen = new Set<string>();
const stringSubstitutions = new Array<[RegExp, string]>();

// Find assets via parameters (for LegacyStackSynthesizer)
const paramRe = /^AssetParameters([a-zA-Z0-9]{64})(S3Bucket|S3VersionKey|ArtifactHash)([a-zA-Z0-9]{8})$/;
for (const paramName of Object.keys(template?.Parameters || {})) {
const m = paramRe.exec(paramName);
if (!m) { continue; }
if (assetsSeen.has(m[1])) { continue; }

assetsSeen.add(m[1]);
const ix = assetsSeen.size;

// Full parameter reference
stringSubstitutions.push([
new RegExp(`AssetParameters${m[1]}(S3Bucket|S3VersionKey|ArtifactHash)([a-zA-Z0-9]{8})`),
`Asset${ix}$1`,
]);
// Substring asset hash reference
stringSubstitutions.push([
new RegExp(`${m[1]}`),
`Asset${ix}Hash`,
]);
}

// find assets defined in the asset manifest
try {
const manifest = AssemblyManifestReader.fromPath(manifestDir);
const assets = manifest.getAssetIdsForStack(stackName);
assets.forEach(asset => {
if (!assetsSeen.has(asset)) {
assetsSeen.add(asset);
const ix = assetsSeen.size;
stringSubstitutions.push([
new RegExp(asset),
`Asset${ix}$1`,
]);
}
});
} catch {
// if there is no asset manifest that is fine.
}

// Substitute them out
return substitute(template);

function substitute(what: any): any {
if (Array.isArray(what)) {
return what.map(substitute);
}

if (typeof what === 'object' && what !== null) {
const ret: any = {};
for (const [k, v] of Object.entries(what)) {
ret[stringSub(k)] = substitute(v);
}
return ret;
}

if (typeof what === 'string') {
return stringSub(what);
}

return what;
}

function stringSub(x: string) {
for (const [re, replacement] of stringSubstitutions) {
x = x.replace(re, replacement);
}
return x;
}
}
}

class StringWritable extends Writable {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ describe('cloud assembly manifest reader', () => {
test('can get assets from assembly manifest', () => {
// WHEN
const manifest = AssemblyManifestReader.fromFile(manifestFile);
const assets = manifest.getAssetsForStack('test-stack2');
const assets = manifest.getAssetLocationsForStack('test-stack2');

// THEN
expect(assets).toEqual([
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,51 @@ describe('IntegTest runSnapshotTests', () => {
}]);
});

test('dont diff new asset hashes', () => {
// WHEN
const integTest = new IntegSnapshotRunner({
cdk: cdkMock.cdk,
test: new IntegTest({
fileName: path.join(__dirname, '../test-data/xxxxx.test-with-new-assets-diff.js'),
discoveryRoot: 'test/test-data',
}),
integOutDir: 'test/test-data/cdk-integ.out.test-with-new-assets',
});
const results = integTest.testSnapshot();
expect(results.diagnostics).toEqual([]);

// THEN
expect(synthFastMock).toHaveBeenCalledTimes(2);
expect(synthFastMock).toHaveBeenCalledWith({
execCmd: ['node', 'xxxxx.test-with-new-assets-diff.js'],
env: expect.objectContaining({
CDK_INTEG_ACCOUNT: '12345678',
CDK_INTEG_REGION: 'test-region',
}),
output: 'cdk-integ.out.test-with-new-assets',
});
});

test('diff new asset hashes', () => {
// WHEN
const integTest = new IntegSnapshotRunner({
cdk: cdkMock.cdk,
test: new IntegTest({
fileName: path.join(__dirname, '../test-data/xxxxx.test-with-new-assets.js'),
discoveryRoot: 'test/test-data',
}),
integOutDir: 'test/test-data/cdk-integ.out.test-with-new-assets-diff',
});
const results = integTest.testSnapshot();

// THEN
expect(results.diagnostics).toEqual(expect.arrayContaining([expect.objectContaining({
reason: DiagnosticReason.SNAPSHOT_FAILED,
testName: integTest.testName,
message: expect.stringContaining('S3Key'),
})]));
});

test('dont diff asset hashes', () => {
// WHEN
const integTest = new IntegSnapshotRunner({
Expand All @@ -146,9 +191,8 @@ describe('IntegTest runSnapshotTests', () => {
}),
integOutDir: 'test/test-data/test-with-snapshot-assets.integ.snapshot',
});
expect(() => {
integTest.testSnapshot();
}).not.toThrow();
const results = integTest.testSnapshot();
expect(results.diagnostics).toEqual([]);

// THEN
expect(synthFastMock).toHaveBeenCalledTimes(2);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"version":"18.0.0"}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"version": "v1.0.0",
"testCases": {
"xxxxx.test-with-new-assets": {
"stacks": ["test-stack"],
"stackUpdateWorkflow": false,
"diffAssets": true,
"allowDestroy": [
"AWS::IAM::Role"
]
}
}
}
Loading