Skip to content

Commit

Permalink
feat(core): acknowledge warnings (aws#26144)
Browse files Browse the repository at this point in the history
This PR adds the ability to acknowledge annotation warning messages. 

The main motivation behind this is to allow people to set the `--strict` mode to fail synthesis on warnings. Currently it is all or nothing, you have to get rid of _all_ warnings to use `--strict`. With this feature users will be able to `acknowledge` warnings saying that they are aware, but it does not apply to them.

Since we want all warnings to now have an id this will deprecate the `addWarning` method and adds a new `addWarningV2` method.

Since the acknowledgements and warnings are written as metadata, it is possible to enhance this in the future to report on warnings and acknowledgements.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
corymhall authored Aug 23, 2023
1 parent dc8f313 commit dd912da
Show file tree
Hide file tree
Showing 89 changed files with 734 additions and 208 deletions.
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-gamelift-alpha/lib/fleet-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -639,10 +639,10 @@ export abstract class FleetBase extends cdk.Resource implements IFleet {
}

protected warnVpcPeeringAuthorizations(scope: Construct): void {
cdk.Annotations.of(scope).addWarning([
cdk.Annotations.of(scope).addWarningV2('@aws-cdk/aws-gamelift:fleetAutorizeVpcPeering', [
'To authorize the VPC peering, call the GameLift service API CreateVpcPeeringAuthorization() or use the AWS CLI command create-vpc-peering-authorization.',
'Make this call using the account that manages your non-GameLift resources.',
'See: https://docs.aws.amazon.com/gamelift/latest/developerguide/vpc-peering.html',
].join('\n'));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ abstract class StackAssociatorBase implements IAspect {
if (Stage.isStage(childNode)) {
var stageAssociated = this.applicationAssociator?.isStageAssociated(childNode);
if (stageAssociated === false) {
this.warning(childNode, 'Associate Stage: ' + childNode.stageName + ' to ensure all stacks in your cdk app are associated with AppRegistry. '
this.warning('StackNotAssociated', childNode, 'Associate Stage: ' + childNode.stageName + ' to ensure all stacks in your cdk app are associated with AppRegistry. '
+ 'You can use ApplicationAssociator.associateStage to associate any stage.');
}
}
Expand Down Expand Up @@ -73,8 +73,8 @@ abstract class StackAssociatorBase implements IAspect {
* @param node The scope to add the warning to.
* @param message The error message.
*/
private warning(node: IConstruct, message: string): void {
Annotations.of(node).addWarning(message);
private warning(id: string, node: IConstruct, message: string): void {
Annotations.of(node).addWarningV2(`@aws-cdk/servicecatalogappregistry:${id}`, message);
}

/**
Expand All @@ -87,12 +87,12 @@ abstract class StackAssociatorBase implements IAspect {
*/
private handleCrossRegionStack(node: Stack): void {
if (isRegionUnresolved(this.application.env.region, node.region)) {
this.warning(node, 'Environment agnostic stack determined, AppRegistry association might not work as expected in case you deploy cross-region or cross-account stack.');
this.warning('EnvironmentAgnosticStack', node, 'Environment agnostic stack determined, AppRegistry association might not work as expected in case you deploy cross-region or cross-account stack.');
return;
}

if (node.region != this.application.env.region) {
this.warning(node, 'AppRegistry does not support cross region associations, deployment might fail if there is cross region stacks in the app. Application region '
this.warning('CrossRegionAssociation', node, 'AppRegistry does not support cross region associations, deployment might fail if there is cross region stacks in the app. Application region '
+ this.application.env.region + ', stack region ' + node.region);
}
}
Expand All @@ -106,7 +106,7 @@ abstract class StackAssociatorBase implements IAspect {
*/
private handleCrossAccountStack(node: Stack): void {
if (isAccountUnresolved(this.application.env.account!, node.account)) {
this.warning(node, 'Environment agnostic stack determined, AppRegistry association might not work as expected in case you deploy cross-region or cross-account stack.');
this.warning('EnvironmentAgnosticStack', node, 'Environment agnostic stack determined, AppRegistry association might not work as expected in case you deploy cross-region or cross-account stack.');
return;
}

Expand All @@ -121,7 +121,7 @@ abstract class StackAssociatorBase implements IAspect {

this.sharedAccounts.add(node.account);
} else {
this.warning(node, 'Cross-account stack detected but application sharing and association will be skipped because cross-account option is not enabled.');
this.warning('AssociationSkipped', node, 'Cross-account stack detected but application sharing and association will be skipped because cross-account option is not enabled.');
return;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ describe('Scope based Associations with Application with Cross Region/Account',
const crossAccountStack = new cdk.Stack(app, 'crossRegionStack', {
env: { account: 'account', region: 'region' },
});
Annotations.fromStack(crossAccountStack).hasWarning('*', 'Cross-account stack detected but application sharing and association will be skipped because cross-account option is not enabled.');
Annotations.fromStack(crossAccountStack).hasWarning('*', 'Cross-account stack detected but application sharing and association will be skipped because cross-account option is not enabled. [ack: @aws-cdk/servicecatalogappregistry:AssociationSkipped]');
});

test('ApplicationAssociator with cross account stacks inside cdkApp does not give warning if associateCrossAccountStacks is set to true', () => {
Expand Down Expand Up @@ -223,7 +223,7 @@ describe('Scope based Associations with Application with Cross Region/Account',
env: { account: 'account', region: 'region' },
});
Annotations.fromStack(crossRegionStack).hasWarning('*', 'AppRegistry does not support cross region associations, deployment might fail if there is cross region stacks in the app.'
+ ' Application region region2, stack region region');
+ ' Application region region2, stack region region [ack: @aws-cdk/servicecatalogappregistry:CrossRegionAssociation]');
});

test('Environment Agnostic ApplicationAssociator with cross region stacks inside cdkApp gives warning', () => {
Expand All @@ -237,7 +237,7 @@ describe('Scope based Associations with Application with Cross Region/Account',
const crossRegionStack = new cdk.Stack(app, 'crossRegionStack', {
env: { account: 'account', region: 'region' },
});
Annotations.fromStack(crossRegionStack).hasWarning('*', 'Environment agnostic stack determined, AppRegistry association might not work as expected in case you deploy cross-region or cross-account stack.');
Annotations.fromStack(crossRegionStack).hasWarning('*', 'Environment agnostic stack determined, AppRegistry association might not work as expected in case you deploy cross-region or cross-account stack. [ack: @aws-cdk/servicecatalogappregistry:EnvironmentAgnosticStack]');
});

test('Cdk App Containing Pipeline with stage but stage not associated throws error', () => {
Expand All @@ -253,7 +253,7 @@ describe('Scope based Associations with Application with Cross Region/Account',
});
app.synth();
Annotations.fromStack(pipelineStack).hasWarning('*',
'Associate Stage: SampleStage to ensure all stacks in your cdk app are associated with AppRegistry. You can use ApplicationAssociator.associateStage to associate any stage.');
'Associate Stage: SampleStage to ensure all stacks in your cdk app are associated with AppRegistry. You can use ApplicationAssociator.associateStage to associate any stage. [ack: @aws-cdk/servicecatalogappregistry:StackNotAssociated]');
});

test('Cdk App Containing Pipeline with stage and stage associated successfully gets synthesized', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ describe('Scope based Associations with Application with Cross Region/Account',
application.associateAllStacksInScope(stage);
Annotations.fromStack(stageStack).hasWarning('*',
'AppRegistry does not support cross region associations, deployment might fail if there is cross region stacks in the app.'
+ ' Application region region, stack region region1');
+ ' Application region region, stack region region1 [ack: @aws-cdk/servicecatalogappregistry:CrossRegionAssociation]');
});
});

Expand Down
17 changes: 9 additions & 8 deletions packages/aws-cdk-lib/assertions/test/annotations.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ describe('Messages', () => {

describe('hasWarning', () => {
test('match', () => {
annotations.hasWarning('/Default/Fred', 'this is a warning');
annotations.hasWarning('/Default/Fred', 'this is a warning [ack: Fred]');
});

test('no match', () => {
Expand All @@ -89,7 +89,7 @@ describe('Messages', () => {
});

test('no match', () => {
expect(() => annotations.hasNoWarning('/Default/Fred', 'this is a warning'))
expect(() => annotations.hasNoWarning('/Default/Fred', 'this is a warning [ack: Fred]'))
.toThrowError(/Expected no matches, but stack has 1 messages as follows:/);
});
});
Expand Down Expand Up @@ -183,16 +183,16 @@ describe('Multiple Messages on the Resource', () => {
test('succeeds on hasXxx APIs', () => {
annotations.hasError('/Default/Foo', 'error: this is an error');
annotations.hasError('/Default/Foo', 'error: unsupported type Foo::Bar');
annotations.hasWarning('/Default/Foo', 'warning: Foo::Bar is deprecated');
annotations.hasWarning('/Default/Foo', 'warning: Foo::Bar is deprecated [ack: Foo]');
});

test('succeeds on findXxx APIs', () => {
const result1 = annotations.findError('*', Match.stringLikeRegexp('error:.*'));
expect(result1.length).toEqual(4);
const result2 = annotations.findError('/Default/Bar', Match.stringLikeRegexp('error:.*'));
expect(result2.length).toEqual(2);
const result3 = annotations.findWarning('/Default/Bar', 'warning: Foo::Bar is deprecated');
expect(result3[0].entry.data).toEqual('warning: Foo::Bar is deprecated');
const result3 = annotations.findWarning('/Default/Bar', 'warning: Foo::Bar is deprecated [ack: Bar]');
expect(result3[0].entry.data).toEqual('warning: Foo::Bar is deprecated [ack: Bar]');
});
});
class MyAspect implements IAspect {
Expand All @@ -209,7 +209,8 @@ class MyAspect implements IAspect {
};

protected warn(node: IConstruct, message: string): void {
Annotations.of(node).addWarning(message);
// Use construct ID as suppression string, just to make it unique easily
Annotations.of(node).addWarningV2(node.node.id, message);
}

protected error(node: IConstruct, message: string): void {
Expand All @@ -231,10 +232,10 @@ class MultipleAspectsPerNode implements IAspect {
}

protected warn(node: IConstruct, message: string): void {
Annotations.of(node).addWarning(message);
Annotations.of(node).addWarningV2(node.node.id, message);
}

protected error(node: IConstruct, message: string): void {
Annotations.of(node).addError(message);
}
}
}
4 changes: 2 additions & 2 deletions packages/aws-cdk-lib/aws-apigateway/lib/method.ts
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ export class Method extends Resource {
public addMethodResponse(methodResponse: MethodResponse): void {
const mr = this.methodResponses.find((x) => x.statusCode === methodResponse.statusCode);
if (mr) {
Annotations.of(this).addWarning(`addMethodResponse called multiple times with statusCode=${methodResponse.statusCode}, deployment will be nondeterministic. Use a single addMethodResponse call to configure the entire response.`);
Annotations.of(this).addWarningV2('@aws-cdk/aws-apigateway:duplicateStatusCodes', `addMethodResponse called multiple times with statusCode=${methodResponse.statusCode}, deployment will be nondeterministic. Use a single addMethodResponse call to configure the entire response.`);
}
this.methodResponses.push(methodResponse);
}
Expand Down Expand Up @@ -512,4 +512,4 @@ export enum AuthorizationType {

function pathForArn(path: string): string {
return path.replace(/\{[^\}]*\}/g, '*'); // replace path parameters (like '{bookId}') with asterisk
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export abstract class Schedule {
public readonly expressionString: string = `cron(${minute} ${hour} ${day} ${month} ${weekDay} ${year})`;
public _bind(scope: Construct) {
if (!options.minute) {
Annotations.of(scope).addWarning('cron: If you don\'t pass \'minute\', by default the event runs every minute. Pass \'minute: \'*\'\' if that\'s what you intend, or \'minute: 0\' to run once per hour instead.');
Annotations.of(scope).addWarningV2('@aws-cdk/aws-applicationautoscaling:defaultRunEveryMinute', 'cron: If you don\'t pass \'minute\', by default the event runs every minute. Pass \'minute: \'*\'\' if that\'s what you intend, or \'minute: 0\' to run once per hour instead.');
}
return new LiteralSchedule(this.expressionString);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,6 @@ export class AutoScalingGroupRequireImdsv2Aspect implements cdk.IAspect {
* @param message The warning message.
*/
protected warn(node: IConstruct, message: string) {
cdk.Annotations.of(node).addWarning(`${AutoScalingGroupRequireImdsv2Aspect.name} failed on node ${node.node.id}: ${message}`);
cdk.Annotations.of(node).addWarningV2(`@aws-cdk/aws-autoscaling:imdsv2${AutoScalingGroupRequireImdsv2Aspect.name}`, `${AutoScalingGroupRequireImdsv2Aspect.name} failed on node ${node.node.id}: ${message}`);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1380,7 +1380,7 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
});

if (desiredCapacity !== undefined) {
Annotations.of(this).addWarning('desiredCapacity has been configured. Be aware this will reset the size of your AutoScalingGroup on every deployment. See https://github.com/aws/aws-cdk/issues/5215');
Annotations.of(this).addWarningV2('@aws-cdk/aws-autoscaling:desiredCapacitySet', 'desiredCapacity has been configured. Be aware this will reset the size of your AutoScalingGroup on every deployment. See https://github.com/aws/aws-cdk/issues/5215');
}

this.maxInstanceLifetime = props.maxInstanceLifetime;
Expand Down Expand Up @@ -2296,7 +2296,7 @@ function synthesizeBlockDeviceMappings(construct: Construct, blockDevices: Block
throw new Error('iops property is required with volumeType: EbsDeviceVolumeType.IO1');
}
} else if (volumeType !== EbsDeviceVolumeType.IO1) {
Annotations.of(construct).addWarning('iops will be ignored without volumeType: EbsDeviceVolumeType.IO1');
Annotations.of(construct).addWarningV2('@aws-cdk/aws-autoscaling:iopsIgnored', 'iops will be ignored without volumeType: EbsDeviceVolumeType.IO1');
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/aws-autoscaling/lib/schedule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export abstract class Schedule {
public readonly expressionString: string = `${minute} ${hour} ${day} ${month} ${weekDay}`;
public _bind(scope: Construct) {
if (!options.minute) {
Annotations.of(scope).addWarning('cron: If you don\'t pass \'minute\', by default the event runs every minute. Pass \'minute: \'*\'\' if that\'s what you intend, or \'minute: 0\' to run once per hour instead.');
Annotations.of(scope).addWarningV2('@aws-cdk/aws-autoscaling:scheduleDefaultRunsEveryMinute', 'cron: If you don\'t pass \'minute\', by default the event runs every minute. Pass \'minute: \'*\'\' if that\'s what you intend, or \'minute: 0\' to run once per hour instead.');
}
return new LiteralSchedule(this.expressionString);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1235,7 +1235,7 @@ describe('auto scaling group', () => {
});

// THEN
Annotations.fromStack(stack).hasWarning('/Default/MyStack', 'iops will be ignored without volumeType: EbsDeviceVolumeType.IO1');
Annotations.fromStack(stack).hasWarning('/Default/MyStack', 'iops will be ignored without volumeType: EbsDeviceVolumeType.IO1 [ack: @aws-cdk/aws-autoscaling:iopsIgnored]');
});

test('warning if iops and volumeType !== IO1', () => {
Expand All @@ -1259,7 +1259,7 @@ describe('auto scaling group', () => {
});

// THEN
Annotations.fromStack(stack).hasWarning('/Default/MyStack', 'iops will be ignored without volumeType: EbsDeviceVolumeType.IO1');
Annotations.fromStack(stack).hasWarning('/Default/MyStack', 'iops will be ignored without volumeType: EbsDeviceVolumeType.IO1 [ack: @aws-cdk/aws-autoscaling:iopsIgnored]');
});

test('step scaling on metric', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ describeDeprecated('scheduled action', () => {
});

// THEN
Annotations.fromStack(stack).hasWarning('/Default/ASG/ScheduledActionScaleOutInTheMorning', "cron: If you don't pass 'minute', by default the event runs every minute. Pass 'minute: '*'' if that's what you intend, or 'minute: 0' to run once per hour instead.");
Annotations.fromStack(stack).hasWarning('/Default/ASG/ScheduledActionScaleOutInTheMorning', "cron: If you don't pass 'minute', by default the event runs every minute. Pass 'minute: '*'' if that's what you intend, or 'minute: 0' to run once per hour instead. [ack: @aws-cdk/aws-autoscaling:scheduleDefaultRunsEveryMinute]");
});

test('scheduled scaling shows no warning when minute is * in cron', () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/aws-cdk-lib/aws-cloudwatch/lib/alarm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,8 @@ export class Alarm extends AlarmBase {
value: props.threshold,
};

for (const w of this.metric.warnings ?? []) {
Annotations.of(this).addWarning(w);
for (const [i, message] of Object.entries(this.metric.warningsV2 ?? {})) {
Annotations.of(this).addWarningV2(i, message);
}
}

Expand Down
11 changes: 8 additions & 3 deletions packages/aws-cdk-lib/aws-cloudwatch/lib/dashboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -179,9 +179,14 @@ export class Dashboard extends Resource {
return;
}

const warnings = allWidgetsDeep(widgets).flatMap(w => w.warnings ?? []);
for (const w of warnings) {
Annotations.of(this).addWarning(w);
const warnings = allWidgetsDeep(widgets).reduce((prev, curr) => {
return {
...prev,
...curr.warningsV2,
};
}, {} as { [id: string]: string });
for (const [id, message] of Object.entries(warnings ?? {})) {
Annotations.of(this).addWarningV2(id, message);
}

const w = widgets.length > 1 ? new Row(...widgets) : widgets[0];
Expand Down
10 changes: 10 additions & 0 deletions packages/aws-cdk-lib/aws-cloudwatch/lib/metric-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,19 @@ export interface IMetric {
* Should be attached to the consuming construct.
*
* @default - None
* @deprecated - use warningsV2
*/
readonly warnings?: string[];

/**
* Any warnings related to this metric
*
* Should be attached to the consuming construct.
*
* @default - None
*/
readonly warningsV2?: { [id: string]: string };

/**
* Inspect the details of the metric object
*/
Expand Down
Loading

0 comments on commit dd912da

Please sign in to comment.