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(core): write Metadata resource in core framework #10306

Merged
merged 18 commits into from
Sep 28, 2020
Merged
Show file tree
Hide file tree
Changes from 2 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
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"5.0.0"}
{"version":"6.0.0"}
19 changes: 15 additions & 4 deletions packages/@aws-cdk/core/lib/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,20 @@ export interface AppProps {
readonly stackTraces?: boolean;

/**
* Include runtime versioning information in cloud assembly manifest
* @default true runtime info is included unless `aws:cdk:disable-runtime-info` is set in the context.
* Include runtime versioning information in the Stacks of this app
*
* @depreacted use `versionReporting` instead
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
* @default Value of 'aws:cdk:version-reporting' context key
*/
readonly runtimeInfo?: boolean;

/**
* Include runtime versioning information in the Stacks of this app
*
* @default Value of 'aws:cdk:version-reporting' context key
*/
readonly versionReporting?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if the correct terminology should be analytics: boolean--broaden the scope a little bit.


/**
* Additional context values for the application.
*
Expand Down Expand Up @@ -101,8 +110,10 @@ export class App extends Stage {
this.node.setContext(cxapi.DISABLE_METADATA_STACK_TRACE, true);
}

if (props.runtimeInfo === false) {
this.node.setContext(cxapi.DISABLE_VERSION_REPORTING, true);
const versionReporting = props.versionReporting ?? props.runtimeInfo;

if (versionReporting !== undefined) {
this.node.setContext(cxapi.VERSION_REPORTING_ENABLED_CONTEXT, versionReporting);
}

const autoSynth = props.autoSynth !== undefined ? props.autoSynth : cxapi.OUTDIR_ENV in process.env;
Expand Down
1 change: 0 additions & 1 deletion packages/@aws-cdk/core/lib/cfn-fn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -752,7 +752,6 @@ class FnJoin implements IResolvable {
}
}


function _inGroupsOf<T>(array: T[], maxGroup: number): T[][] {
const result = new Array<T[]>();
for (let i = 0; i < array.length; i += maxGroup) {
Expand Down
1 change: 0 additions & 1 deletion packages/@aws-cdk/core/lib/private/metadata-resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ function formatModules(runtime: cxapi.RuntimeInfo): string {
const modules = new Array<string>();

// inject toolkit version to list of modules
// eslint-disable-next-line @typescript-eslint/no-require-imports
const cliVersion = process.env[cxapi.CLI_VERSION_ENV];
if (cliVersion) {
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
modules.push(`aws-cdk=${cliVersion}`);
Expand Down
13 changes: 4 additions & 9 deletions packages/@aws-cdk/core/lib/private/synthesis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,20 +129,15 @@ function prepareTree(root: IConstruct) {
*/
function injectMetadataResources(root: IConstruct) {
visit(root, 'post', construct => {
// Only on top-level stacks
if (!Stack.isStack(construct)
|| construct.parentStack
// Unless disabled
|| construct.node.tryGetContext(cxapi.DISABLE_VERSION_REPORTING)
// While running via CLI
|| !process.env[cxapi.CLI_VERSION_ENV]) { return; }
if (!Stack.isStack(construct) || !construct._versionReportingEnabled) { return; }

// Because of https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/assert/lib/synth-utils.ts#L74
// synthesize() may be called more than once on a stack in unit tests, and the below would break
// if we execute it a second time. Guard against the constructs already existing.
if (construct.node.tryFindChild('CDKMetadata')) { return; }
const CDKMetadata = 'CDKMetadata';
if (construct.node.tryFindChild(CDKMetadata)) { return; }

new MetadataResource(construct, 'CDKMetadata');
new MetadataResource(construct, CDKMetadata);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remind me what's the rationale for not just adding this in the Stack's constructor based on the prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't set context anymore

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And why do we need to set context? My view is that the context is just used to determine the default for analyticsReporting when new stacks are created. From that point forward we shouldn't care about the context key, no?

});
}

Expand Down
21 changes: 21 additions & 0 deletions packages/@aws-cdk/core/lib/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,14 @@ export interface StackProps {
* @default false
*/
readonly terminationProtection?: boolean;

/**
* Include runtime versioning information in this Stack
*
* @default `versionReporting setting of containing `App`, or value of
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
* 'aws:cdk:version-reporting' context key
*/
readonly versionReporting?: boolean;
}

/**
Expand Down Expand Up @@ -279,6 +287,15 @@ export class Stack extends Construct implements ITaggable {
*/
public readonly synthesizer: IStackSynthesizer;

/**
* Whether version reporting is enabled for this stack
*
* Controls whether the CDK Metadata resource is injected
*
* @internal
*/
public readonly _versionReportingEnabled: boolean;

/**
* Logical ID generation strategy
*/
Expand Down Expand Up @@ -368,6 +385,10 @@ export class Stack extends Construct implements ITaggable {

this.templateFile = `${this.artifactId}.template.json`;

// Not for nested stacks
this._versionReportingEnabled = (props.versionReporting ?? this.node.tryGetContext(cxapi.VERSION_REPORTING_ENABLED_CONTEXT))
&& !this.nestedStackParent;

this.synthesizer = props.synthesizer ?? (newStyleSynthesisContext
? new DefaultStackSynthesizer()
: new LegacyStackSynthesizer());
Expand Down
77 changes: 43 additions & 34 deletions packages/@aws-cdk/core/test/test.app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@ import * as cxapi from '@aws-cdk/cx-api';
import { Test } from 'nodeunit';
import { CfnResource, Construct, Stack, StackProps } from '../lib';
import { Annotations } from '../lib/annotations';
import { MetadataResource } from '../lib/private/metadata-resource';
import { App, AppProps } from '../lib/app';
import { MetadataResource } from '../lib/private/metadata-resource';

function withApp(props: AppProps, block: (app: App) => void): cxapi.CloudAssembly {
const app = new App({
runtimeInfo: false,
stackTraces: false,
...props,
});
Expand Down Expand Up @@ -252,24 +251,40 @@ export = {
* The are not emitted into Cloud Assembly metadata anymore
*/
'runtime library versions are not emitted in asm anymore'(test: Test) {
withCliVersion(() => {
const context: any = {};
const assembly = withApp({ versionReporting: true }, app => {
const stack = new Stack(app, 'stack1');
new CfnResource(stack, 'MyResource', { type: 'Resource::Type' });
});

const assembly = withApp(context, app => {
const stack = new Stack(app, 'stack1');
new CfnResource(stack, 'MyResource', { type: 'Resource::Type' });
});
test.deepEqual(assembly.runtime, { libraries: {} });
test.done();
},

test.deepEqual(assembly.runtime, { libraries: {} });
'runtime library versions'(test: Test) {
MetadataResource.clearModulesCache();

const response = withApp({ versionReporting: true }, app => {
const stack = new Stack(app, 'stack1');
new CfnResource(stack, 'MyResource', { type: 'Resource::Type' });
});

const stackTemplate = response.getStackByName('stack1').template;
const libs = parseModules(stackTemplate.Resources?.CDKMetadata?.Properties?.Modules);

// eslint-disable-next-line @typescript-eslint/no-require-imports
const version = require('../package.json').version;
test.deepEqual(libs['@aws-cdk/core'], version);
test.deepEqual(libs['@aws-cdk/cx-api'], version);
test.deepEqual(libs['jsii-runtime'], `node.js/${process.version}`);

test.done();
},

'runtime library versions'(test: Test) {
'CDK version'(test: Test) {
MetadataResource.clearModulesCache();

withCliVersion(() => {
const response = withApp({ runtimeInfo: true }, app => {
const response = withApp({ versionReporting: true }, app => {
const stack = new Stack(app, 'stack1');
new CfnResource(stack, 'MyResource', { type: 'Resource::Type' });
});
Expand All @@ -278,10 +293,7 @@ export = {
const libs = parseModules(stackTemplate.Resources?.CDKMetadata?.Properties?.Modules);

// eslint-disable-next-line @typescript-eslint/no-require-imports
const version = require('../package.json').version;
test.deepEqual(libs['@aws-cdk/core'], version);
test.deepEqual(libs['@aws-cdk/cx-api'], version);
test.deepEqual(libs['jsii-runtime'], `node.js/${process.version}`);
test.deepEqual(libs['aws-cdk'], '1.2.3');
});

test.done();
Expand All @@ -292,7 +304,7 @@ export = {
MetadataResource.clearModulesCache();

withCliVersion(() => {
const response = withApp({ runtimeInfo: true }, app => {
const response = withApp({ versionReporting: true }, app => {
const stack = new Stack(app, 'stack1');
new CfnResource(stack, 'MyResource', { type: 'Resource::Type' });
});
Expand All @@ -309,27 +321,24 @@ export = {

'version reporting includes only @aws-cdk, aws-cdk and jsii libraries'(test: Test) {
MetadataResource.clearModulesCache();
withCliVersion(() => {
const response = withApp({ runtimeInfo: true }, app => {
const stack = new Stack(app, 'stack1');
new CfnResource(stack, 'MyResource', { type: 'Resource::Type' });
});

const stackTemplate = response.getStackByName('stack1').template;
const libs = parseModules(stackTemplate.Resources?.CDKMetadata?.Properties?.Modules);

// eslint-disable-next-line @typescript-eslint/no-require-imports
const version = require('../package.json').version;
test.deepEqual(libs, {
'aws-cdk': '1.2.3',
'@aws-cdk/core': version,
'@aws-cdk/cx-api': version,
'@aws-cdk/region-info': version,
'@aws-cdk/cloud-assembly-schema': version,
'jsii-runtime': `node.js/${process.version}`,
});
const response = withApp({ versionReporting: true }, app => {
const stack = new Stack(app, 'stack1');
new CfnResource(stack, 'MyResource', { type: 'Resource::Type' });
});

const stackTemplate = response.getStackByName('stack1').template;
const libs = parseModules(stackTemplate.Resources?.CDKMetadata?.Properties?.Modules);
const libNames = Object.keys(libs).sort();

test.deepEqual(libNames, [
'@aws-cdk/cloud-assembly-schema',
'@aws-cdk/core',
'@aws-cdk/cx-api',
'@aws-cdk/region-info',
'jsii-runtime',
]);

test.done();
},

Expand Down
18 changes: 18 additions & 0 deletions packages/@aws-cdk/core/test/test.stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -975,6 +975,24 @@ export = {

test.done();
},

'version reporting can be configured on the app'(test: Test) {
const app = new App({ versionReporting: true });
test.ok(new Stack(app, 'Stack')._versionReportingEnabled);
test.done();
},

'version reporting can be configured with context'(test: Test) {
const app = new App({ context: { 'aws:cdk:version-reporting': true } });
test.ok(new Stack(app, 'Stack')._versionReportingEnabled);
test.done();
},

'version reporting can be configured on the stack'(test: Test) {
const app = new App();
test.ok(new Stack(app, 'Stack', { versionReporting: true })._versionReportingEnabled);
test.done();
},
};

class StackWithPostProcessor extends Stack {
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/core/test/test.stage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ export = {

'Assemblies can be deeply nested'(test: Test) {
// GIVEN
const app = new App({ runtimeInfo: false, treeMetadata: false });
const app = new App({ treeMetadata: false });

const level1 = new Stage(app, 'StageLevel1');
const level2 = new Stage(level1, 'StageLevel2');
Expand Down
11 changes: 1 addition & 10 deletions packages/@aws-cdk/core/test/util.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,6 @@
import * as cxapi from '@aws-cdk/cx-api';
import { Stack, Construct, StackProps, App } from '../lib';
import { Stack } from '../lib';
import { synthesize } from '../lib/private/synthesis';

export class TestStack extends Stack {
constructor(scope: Construct, id: string, props?: StackProps) {
super(scope ?? new App({
context: { [cxapi.DISABLE_VERSION_REPORTING]: true },
}), id, props);
}
}

export function toCloudFormation(stack: Stack): any {
return synthesize(stack, { skipValidation: true }).getStackByName(stack.stackName).template;
}
Expand Down
7 changes: 7 additions & 0 deletions packages/@aws-cdk/cx-api/lib/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,16 @@ export const PATH_METADATA_ENABLE_CONTEXT = 'aws:cdk:enable-path-metadata';

/**
* Disable the collection and reporting of version information.
*
* @deprecated Use ENABLE_VERSION_REPORTING instead
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
*/
export const DISABLE_VERSION_REPORTING = 'aws:cdk:disable-version-reporting';

/**
* Enable the collection and reporting of version information.
*/
export const VERSION_REPORTING_ENABLED_CONTEXT = 'aws:cdk:version-reporting';

/**
* If this is set, asset staging is disabled. This means that assets will not be copied to
* the output directory and will be referenced with absolute source paths.
Expand Down
1 change: 0 additions & 1 deletion packages/@aws-cdk/pipelines/test/testutil.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ export class TestApp extends App {
},
stackTraces: false,
autoSynth: false,
runtimeInfo: false,
treeMetadata: false,
...props,
});
Expand Down
10 changes: 3 additions & 7 deletions packages/aws-cdk/lib/api/cxapp/exec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,21 @@ export async function execProgram(aws: SdkProvider, config: Configuration): Prom
await populateDefaultEnvironmentIfNeeded(aws, env);

const pathMetadata: boolean = config.settings.get(['pathMetadata']) ?? true;

if (pathMetadata) {
context[cxapi.PATH_METADATA_ENABLE_CONTEXT] = true;
}

const assetMetadata: boolean = config.settings.get(['assetMetadata']) ?? true;

if (assetMetadata) {
context[cxapi.ASSET_RESOURCE_METADATA_ENABLED_CONTEXT] = true;
}

const versionReporting: boolean = config.settings.get(['versionReporting']) ?? true;

if (!versionReporting) {
context[cxapi.DISABLE_VERSION_REPORTING] = true;
}
if (versionReporting) { context[cxapi.VERSION_REPORTING_ENABLED_CONTEXT] = true; }
// We need to keep on doing this for framework version from before this flag was deprecated.
if (!versionReporting) { context[cxapi.DISABLE_VERSION_REPORTING] = true; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can remove this constant from cxapi and just plug the explicit value here for backwards compat

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh please yes


const stagingEnabled = config.settings.get(['staging']) ?? true;

if (!stagingEnabled) {
context[cxapi.DISABLE_ASSET_STAGING_CONTEXT] = true;
}
Expand Down