Need a second opinion on this

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.

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

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

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?

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]

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();
  }
}

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?