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

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

Closed
1 of 2 tasks
cayman-amzn opened this issue Dec 20, 2022 · 7 comments
Closed
1 of 2 tasks

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

cayman-amzn opened this issue Dec 20, 2022 · 7 comments
Labels
@aws-cdk/core Related to core CDK functionality effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1

Comments

@cayman-amzn
Copy link

Describe the feature

The idea here would be to create a commitable lock file of generated annotations, a similar concept to NPM's pacakge-lock.json.

  • "Snapshots" of annotations, allowing users to acknowledge in a PR/CR that you are suppressing a suggestion or warning from an annotation.
    • This also allows you to see when something changes between construct versions.
  • Another way to reduce noise in the console during builds/synths

Use Case

export class CustomQueue extends Construct {
  readonly queue: Queue;

  private readonly VISIBILITY_TIMEOUT_MAX = Duration.minutes(10);

  constructor(scope: Construct, id: string, props?: QueueProps) {
    super(scope, id);

    this.queue = new Queue(this, "TheQueue", props);

    if (
      props?.visibilityTimeout &&
      props.visibilityTimeout > this.VISIBILITY_TIMEOUT_MAX
    ) {
      // Generates a warning in the console like:
      // [Warning at /CdkStack/MyOrders/TheQueue] A visibilityTimeout value of less than 10 minutes is recommended for the CustomQueue construct

      Annotations.of(this.queue).addWarning(
        `A visibilityTimeout value of less than ${this.VISIBILITY_TIMEOUT_MAX.toHumanString()} is recommended for the CustomQueue construct`
      );
    }
  }
}

Proposed Solution

When this lock file is committed, then we would no longer print out the warning in the console. If the annotation, level or scope have changed, then that would render the locked annotation out of date and would no longer suppress the message (We could probably do this with some base64 encoding?)

Flat option:

// <project-root>/cdk-annotations-lock.jsonc
[
  {
    "scope": "/CdkStack/MyOrders/TheQueue",
    "annotation": "A visibilityTimeout value of less than 10 minutes is recommended for the CustomQueue construct",
    "level": "warning",
    "suppressedAt": "2022-12-20T03:52:24+00:00"
  }
]

Nested Option:

// <project-root>/cdk-annotations-lock.jsonc
{
  "CdkStack": {
    "MyOrders": {
      "TheQueue": {
        "annotation": "A visibilityTimeout value of less than 10 minutes is recommended for the CustomQueue construct",
        "level": "warning",
        "suppressedAt": "2022-12-20T03:52:24+00:00"
      }
    }
  }
}

I purposefully use jsonc here to ensure users know they can put comments in the lock file. That way, if you wanted to create a task or backlog item in a ticketing system you could reference it here.

Other Information

This is definitely just an idea that I thought of after using CDK for the past year or so, with a particularly noisy construct library. Absolutely open to discussion/thoughts on the idea or if there are better ways to do this.

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.55.1

Environment details (OS name and version, etc.)

OS independant

@cayman-amzn cayman-amzn added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Dec 20, 2022
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Dec 20, 2022
@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 4, 2023

This is an excellent idea that's been in the back of my mind as well. The flat option should be simple enough to implement.

@rix0rrr rix0rrr added effort/small Small work item – less than a day of effort p1 and removed needs-triage This issue or PR still needs to be triaged. labels Jan 4, 2023
@rix0rrr rix0rrr removed their assignment Jan 4, 2023
@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 8, 2023

Here is my design for this feature, I don't have time to implement it but maybe someone else will.

The steps are:

  • addWarning gets an extra key
  • Warnings can be suppressed from code, and from context

1. addWarning gets an extra key

      Annotations.of(this.queue).addWarning('whatever the message', {
         suppressionKey: 'max-visibility',
      });

2. Warnings can be suppressed from code

  Annotations.of(this.queue).suppressWarning(
    'max-visibility',
    'something-else',
  );

Will suppress all warnings underneath the indicated construct, with the given key. It should work before and after the construct has been added to the tree (so Annotations.of(this).suppressWarning(...) will also work).

3. Warnings can be suppressed from context

// cdk.json
{
  "app": "ts-node ...",
  "context": {
    "suppress:/Stack/Construct/Queue:max-visibility
  }
}

Effect should be the same as putting Annotations.of(thatQueue).suppressWarning('max-visibility') in the source. That is: all warnings of the given type emanating from Queue or any construct under it should be suppressed.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 8, 2023

Potentially there's a key * we could use to also suppress warnings that don't have a suppressionkey.

@cayman-amzn
Copy link
Author

I like your idea @rix0rrr

What do you think about automatically generating the suppression key? I'm thinking like how resource id's are generated (logical/physical). Then we could add to the Annotation logging to include that key

Also, we should probably have some sort of toggle for Construct owners to enable/disable an annotation from being suppressed.

@m-radzikowski
Copy link

I believe this is implemented now with cdk.Annotations.of().acknowledgeWarning()? Warning now include ack and can be suppressed. Working example:

Warning:

[Warning at /MyStack/VPC/PublicSubnet1] No routeTableId was provided to the subnet at 'MyStack/VPC/PublicSubnet1'. Attempting to read its .routeTable.routeTableId will return null/undefined. (More info: https://github.com/aws/aws-cdk/pull/3171) [ack: @aws-cdk/aws-ec2:noSubnetRouteTableId]

Suppression:

cdk.Annotations.of(vpc).acknowledgeWarning("@aws-cdk/aws-ec2:noSubnetRouteTableId");

@cayman-amzn
Copy link
Author

Yeah, it does seem like we've solved the problem with #26144 :D

Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

No branches or pull requests

3 participants