feat(core): acknowledge warnings #26144

corymhall · 2023-06-28T11:42:01Z

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

This PR adds the ability to acknowledge annotation warning messages. I've updated some of the annotations in the `rds.Cluster` construct to show how this will work. After this is merged we can work on adding the ids to all of the warning messages throughout the construct library. 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.

gitpod-io · 2023-06-28T11:42:06Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

corymhall · 2023-06-29T18:55:28Z

allowed-breaking-changes.txt

@@ -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


Need a second opinion on this

rix0rrr

Thanks for this, this will be a great improvement!

My overall concerns are:

I'd like to do the filtering inside the CDK app and not change the cloud assembly format.
Quite personal, but I don't like the format of the suppression flags. I'd personally prefer camelCase or kebab-case, and not too many components to it.

The last one can be argued though, there is no accounting for taste I guess. But maybe put it up to the team with a couple of alternatives?

rix0rrr · 2023-06-30T11:21:40Z

packages/@aws-cdk/aws-gamelift-alpha/lib/fleet-base.ts

@@ -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', [


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 ?

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

packages/@aws-cdk/aws-servicecatalogappregistry-alpha/test/application-associator.test.ts

packages/aws-cdk-lib/assertions/test/annotations.test.ts

rix0rrr · 2023-06-30T11:28:54Z

packages/aws-cdk-lib/cloud-assembly-schema/lib/cloud-assembly/metadata-schema.ts

+/**
+ * @see ArtifactMetadataEntryType.ACKNOWLEDGE
+ */
+export interface AcknowledgementMetadataEntry {


Why do acknowledgements need to make it into the Cloud Assembly? Can't we choose to not emit acknowledged warnings?

rix0rrr · 2023-06-30T11:30:37Z

packages/aws-cdk-lib/core/lib/annotations.ts

+  public addWarningV2(id: string, message: string) {
+    const warning = {
+      id: id,
+      scope: this.scope.node.path,


Metadata messages are already attached to a scope, so this information will be doubled?

rix0rrr · 2023-06-30T11:31:54Z

packages/aws-cdk-lib/core/test/annotations.test.ts

+      for (const [_path, md] of Object.entries(s.metadata ?? {})) {
+        for (const x of md) {
+          if (x.type === 'aws:cdk:acknowledge') {
+            acknoledgement.message = x.data as string;


Mutations in the THEN part of a test?

🤦‍♂️

Just asking, if you need to do this work it probably wants to go into a reusable function that's in the core library?

rix0rrr · 2023-06-30T11:33:39Z

packages/aws-cdk-lib/core/test/app.test.ts

@@ -78,8 +78,8 @@ describe('app', () => {
      '/stack1/s1c1': [{ type: 'aws:cdk:logicalId', data: 's1c1' }],
      '/stack1/s1c2':
        [{ type: 'aws:cdk:logicalId', data: 's1c2' },
-          { type: 'aws:cdk:warning', data: 'warning1' },
-          { type: 'aws:cdk:warning', data: 'warning2' }],
+          { type: 'aws:cdk:warning', data: { id: 'warning1', message: 'warning1', scope: 'stack1/s1c2' } },


This changes the type associated with aws:cdk:warning. I don't think that's safe if we consider external processors (like SST undoubtedly does).

Either we now have a new type aws:cdk:warningv2, or we find a way to add the new information additively into the old schema.

But if we can keep warning filtering entirely in the CDK app, there's no need to change this format at all. We can turn the message of a warningv2 into:

{ type: 'aws:cdk:warning', data: '[warning1] warning message here' }

And it'll be backwards compatible.

Or honestly, a better message would probably be:

warning message here [warning1]

rix0rrr · 2023-06-30T11:35:36Z

packages/aws-cdk-lib/cx-api/lib/cloud-artifact.ts

+        if (entry.type === cxschema.ArtifactMetadataEntryType.ACKNOWLEDGE) {
+          const data = (entry.data as cxschema.AcknowledgementMetadataEntry);
+          acks.set(data.id, data.scopes);
+        }


I think all of this work should probably happen at synth time in the CDK app.

I started going down that path and ended up with this solution for a couple of reasons, the main one being that we create a new instance of Annotations every time we create a message. We currently use the metadata as the storage mechanism. In order to do it at synth time we would need to implement a new storage mechanism. This probably wouldn't be too bad (probably just another construct we getOrCreate).

I can explore that.

Here's what we could do:

acknowledgeWarning() will:

Set context which will prevent warnings with that ID being set in the future

Will remove any existing metadata warnings with that ID right now

That would achieve it?

We can't set context after constructs have been added.

What about something like this (pseudo code)

class AnnotationManager extends Construct { private constructor(scope: Construct) { attachCustomSynthesis(this, { onSynthesize: () => { this.warnings.forEach(warning => node.addMetadata(...)) } } } public addWarning() { if (!this.acks.has()) { this.warnings.set(); } } public ack() { if (this.warnings.has()) { this.warnings.delete(); } this.acks.add(); } }

rix0rrr · 2023-06-30T14:26:50Z

We can't set context after constructs have been added.

Honestly, I've seen this restriction do more harm than good.

I'm not opposed to removing it.

rix0rrr · 2023-07-06T09:31:58Z

packages/aws-cdk-lib/core/lib/adr/acknowledge-warnings.md

+With the recommended solution the only major consequence is that it requires
+updating the `metadata-schema`, but we can do this in a non-breaking way
+(addition of new types). The alternatives may also require changes to the schema


It changes the schema, and interpreting the schema becomes more complex. I would really really like to avoid having an add list and a scrap list in the same piece of data, that the client hen has to evaluate.

Adding this as a feature to the core seems preferable to me.

@rix0rrr Then do you think the context + remove metadata option is the one to go with?

…s-cdk into pr/corymhall/26144

mergify · 2023-08-23T11:29:45Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2023-08-23T12:08:10Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-08-23T12:37:13Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: e8d78d6
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-08-23T12:37:37Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation requested a review from a team June 28, 2023 11:42

github-actions bot added the p2 label Jun 28, 2023

mergify bot added the contribution/core This is a PR that came from AWS. label Jun 28, 2023

adding deprecation

fca3a1a

aws-cdk-automation previously requested changes Jun 28, 2023

View reviewed changes

corymhall marked this pull request as draft June 28, 2023 11:45

corymhall added 3 commits June 29, 2023 13:26

updating all references from addWarning to addWarningV2

707be7f

updated readme

5229dc9

fixing lint errors

bb217e3

corymhall added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jun 29, 2023

corymhall marked this pull request as ready for review June 29, 2023 14:53

corymhall added 3 commits June 29, 2023 16:35

fixing tests

9e19207

Merge branch 'main' into corymhall/core/ack-warnings

a9f506e

adding to breaking change

4b8ad28

corymhall commented Jun 29, 2023

View reviewed changes

couple more

eb244d8

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 29, 2023

adding examples

54a2910

rix0rrr requested changes Jun 30, 2023

View reviewed changes

rix0rrr changed the title ~~feat(core): acknowledge annotation warning messages~~ feat(core): acknowledge warnings Jun 30, 2023

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 30, 2023

corymhall added 2 commits July 5, 2023 14:32

adding adr

06f8559

formatting the messages

4af1135

rix0rrr reviewed Jul 6, 2023

View reviewed changes

corymhall marked this pull request as draft July 6, 2023 10:07

MrArnoldPalmer mentioned this pull request Aug 16, 2023

(core): stateful annotations #26778

Open

2 tasks

rix0rrr added 3 commits August 18, 2023 15:25

Update adr

df8a853

Undo test change

5aba9ac

addWarning -> addWarningV2

ee9fc76

rix0rrr marked this pull request as ready for review August 18, 2023 13:32

rix0rrr added 10 commits August 18, 2023 15:33

Linterrrr

56625ac

Update tests

cd72788

More tests

87b3b12

Oops

c0b5319

Test again

a95dde9

Merge branch 'main' into corymhall/core/ack-warnings

acca007

Foutje

da5211d

Merge branch 'corymhall/core/ack-warnings' of github.com:corymhall/aw…

9e9a6fd

…s-cdk into pr/corymhall/26144

Merge branch 'main' into corymhall/core/ack-warnings

52ecb54

Merge branch 'corymhall/core/ack-warnings' of github.com:corymhall/aw…

7c5b1c7

…s-cdk into pr/corymhall/26144

rix0rrr approved these changes Aug 22, 2023

View reviewed changes

rix0rrr added 4 commits August 22, 2023 13:43

This is not a warning

4fb2770

Merge branch 'main' into corymhall/core/ack-warnings

af14a3f

Exclude "__proto__" from fast-check

696945d

Fix examples

6361023

Merge branch 'main' into corymhall/core/ack-warnings

52eab30

Merge branch 'main' into corymhall/core/ack-warnings

e8d78d6

mergify bot merged commit dd912da into aws:main Aug 23, 2023
9 checks passed

mrgrain mentioned this pull request Aug 23, 2023

feat(lambda): L2 constructs for SnapStart #26761

Merged

JohannesKonings mentioned this pull request Aug 26, 2023

switch from Annotations addWarning to addWarningV2 cdklabs/cdk-nag#1418

Open

dontirun mentioned this pull request Aug 28, 2023

(core): addUnacknowledgeableWarning() method for Annotations #26914

Open

2 tasks

cayman-amzn mentioned this pull request Nov 15, 2023

(core): Add support for suppressing Annotation messages #23403

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): acknowledge warnings #26144

feat(core): acknowledge warnings #26144

corymhall commented Jun 28, 2023 •

edited

gitpod-io bot commented Jun 28, 2023

aws-cdk-automation left a comment

corymhall Jun 29, 2023

rix0rrr left a comment

rix0rrr Jun 30, 2023

corymhall Jun 30, 2023

rix0rrr Jun 30, 2023

rix0rrr Jun 30, 2023

rix0rrr Jun 30, 2023

corymhall Jun 30, 2023

rix0rrr Jun 30, 2023

rix0rrr Jun 30, 2023 •

edited

rix0rrr Jun 30, 2023

rix0rrr Jun 30, 2023

corymhall Jun 30, 2023

rix0rrr Jun 30, 2023

corymhall Jun 30, 2023

rix0rrr commented Jun 30, 2023

rix0rrr Jul 6, 2023

corymhall Jul 6, 2023

mergify bot commented Aug 23, 2023

mergify bot commented Aug 23, 2023

aws-cdk-automation commented Aug 23, 2023

mergify bot commented Aug 23, 2023

feat(core): acknowledge warnings #26144

feat(core): acknowledge warnings #26144

Conversation

corymhall commented Jun 28, 2023 • edited

gitpod-io bot commented Jun 28, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Jun 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Jun 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Aug 23, 2023

mergify bot commented Aug 23, 2023

aws-cdk-automation commented Aug 23, 2023

AWS CodeBuild CI Report

mergify bot commented Aug 23, 2023

corymhall commented Jun 28, 2023 •

edited

rix0rrr Jun 30, 2023 •

edited