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

feat(core): acknowledge warnings #26144

Merged
merged 35 commits into from
Aug 23, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
5ce3b70
feat(core): acknowledge annotation warning messages
corymhall Jun 27, 2023
fca3a1a
adding deprecation
corymhall Jun 28, 2023
707be7f
updating all references from `addWarning` to `addWarningV2`
corymhall Jun 29, 2023
5229dc9
updated readme
corymhall Jun 29, 2023
bb217e3
fixing lint errors
corymhall Jun 29, 2023
9e19207
fixing tests
corymhall Jun 29, 2023
a9f506e
Merge branch 'main' into corymhall/core/ack-warnings
corymhall Jun 29, 2023
4b8ad28
adding to breaking change
corymhall Jun 29, 2023
eb244d8
couple more
corymhall Jun 29, 2023
54a2910
adding examples
corymhall Jun 30, 2023
06f8559
adding adr
corymhall Jul 5, 2023
4af1135
formatting the messages
corymhall Jul 5, 2023
9198add
Merge remote-tracking branch 'origin/main' into pr/corymhall/26144
rix0rrr Aug 18, 2023
957c238
Keep warning suppression synth-side by means of an App global
rix0rrr Aug 18, 2023
f0ce23b
Don't need these breaking changes anymore
rix0rrr Aug 18, 2023
6412029
Comment
rix0rrr Aug 18, 2023
df8a853
Update adr
rix0rrr Aug 18, 2023
5aba9ac
Undo test change
rix0rrr Aug 18, 2023
ee9fc76
addWarning -> addWarningV2
rix0rrr Aug 18, 2023
56625ac
Linterrrr
rix0rrr Aug 18, 2023
cd72788
Update tests
rix0rrr Aug 21, 2023
87b3b12
More tests
rix0rrr Aug 21, 2023
c0b5319
Oops
rix0rrr Aug 21, 2023
a95dde9
Test again
rix0rrr Aug 21, 2023
acca007
Merge branch 'main' into corymhall/core/ack-warnings
rix0rrr Aug 21, 2023
da5211d
Foutje
rix0rrr Aug 22, 2023
9e9a6fd
Merge branch 'corymhall/core/ack-warnings' of github.com:corymhall/aw…
rix0rrr Aug 22, 2023
52ecb54
Merge branch 'main' into corymhall/core/ack-warnings
rix0rrr Aug 22, 2023
7c5b1c7
Merge branch 'corymhall/core/ack-warnings' of github.com:corymhall/aw…
rix0rrr Aug 22, 2023
4fb2770
This is not a warning
rix0rrr Aug 22, 2023
af14a3f
Merge branch 'main' into corymhall/core/ack-warnings
rix0rrr Aug 23, 2023
696945d
Exclude "__proto__" from fast-check
rix0rrr Aug 23, 2023
6361023
Fix examples
rix0rrr Aug 23, 2023
52eab30
Merge branch 'main' into corymhall/core/ack-warnings
rix0rrr Aug 23, 2023
e8d78d6
Merge branch 'main' into corymhall/core/ack-warnings
rix0rrr Aug 23, 2023
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
4 changes: 4 additions & 0 deletions allowed-breaking-changes.txt
Original file line number Diff line number Diff line change
Expand Up @@ -164,3 +164,7 @@ removed:aws-cdk-lib.triggers.TriggerProps.timeout
change-return-type:aws-cdk-lib.aws_ec2.SecurityGroup.determineRuleScope
# broken only in non-JS/TS, where that was not previously usable
strengthened:aws-cdk-lib.aws_s3_deployment.BucketDeploymentProps

# weakened added additional types to a union type, meaning all new props are optional
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need a second opinion on this

weakened:aws-cdk-lib.cloud_assembly_schema.MetadataEntry
weakened:aws-cdk-lib.cx_api.MetadataEntryResult
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('Gamelift:Fleet:AutorizeVpcPeering', [
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to schematize this, let's set a standard? This one uses a 3-part warning, the one below uses a 2-part warning.

If we take a cue from the feature flags, which this feels a bit similar to, maybe the flag format is something like @aws-cdk/aws-service:someCamelCaseTerm ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I like this format better @aws-cdk/aws-service:someCamelCaseTerm. I'll update.

'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(`SCAppRegistry:${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('*', 'SCAppRegistry:AssociationSkipped: Cross-account stack detected but application sharing and association will be skipped because cross-account option is not enabled.');
});

test('ApplicationAssociator with cross account stacks inside cdkApp does not give warning if associateCrossAccountStacks is set to true', () => {
Expand Down Expand Up @@ -222,7 +222,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('*', 'AppRegistry does not support cross region associations, deployment might fail if there is cross region stacks in the app.'
Annotations.fromStack(crossRegionStack).hasWarning('*', 'SCAppRegistry:CrossRegionAssociation: 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');
});

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('*', 'SCAppRegistry:EnvironmentAgnosticStack: Environment agnostic stack determined, AppRegistry association might not work as expected in case you deploy cross-region or cross-account stack.');
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
});

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.');
'SCAppRegistry:StackNotAssociated: Associate Stage: SampleStage to ensure all stacks in your cdk app are associated with AppRegistry. You can use ApplicationAssociator.associateStage to associate any stage.');
});

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 @@ -511,7 +511,7 @@ describe('Scope based Associations with Application with Cross Region/Account',
const stageStack = new cdk.Stack(stage, 'MyStack');
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.'
'SCAppRegistry:CrossRegionAssociation: 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');
});
});
Expand Down
16 changes: 8 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', 'Fred: this is a warning');
});

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', 'Fred: this is a warning'))
.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', 'Foo: warning: Foo::Bar is deprecated');
});

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', 'Bar: warning: Foo::Bar is deprecated');
expect(result3[0].entry.data).toEqual('Bar: warning: Foo::Bar is deprecated');
});
});
class MyAspect implements IAspect {
Expand All @@ -209,7 +209,7 @@ class MyAspect implements IAspect {
};

protected warn(node: IConstruct, message: string): void {
Annotations.of(node).addWarning(message);
Annotations.of(node).addWarningV2(node.node.id, message);
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
}

protected error(node: IConstruct, message: string): void {
Expand All @@ -231,10 +231,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);
}
}
}
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('AppAutoScaling:Schedule: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(`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 @@ -1353,7 +1353,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('AutoScaling:Group: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 @@ -2266,7 +2266,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('AutoScaling:Group: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('AutoScaling:Schedule:DefaultRunsEveryMinute', '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 @@ -1025,7 +1025,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', 'AutoScaling:Group:IopsIgnored: iops will be ignored without volumeType: EbsDeviceVolumeType.IO1');
});

test('warning if iops and volumeType !== IO1', () => {
Expand All @@ -1049,7 +1049,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', 'AutoScaling:Group:IopsIgnored: iops will be ignored without volumeType: EbsDeviceVolumeType.IO1');
});

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', "AutoScaling:Schedule:DefaultRunsEveryMinute: 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.");
});

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 @@ -163,9 +163,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