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

feat(core): acknowledge warnings #26144

merged 35 commits into from
Aug 23, 2023

Conversation

corymhall
Copy link
Contributor

@corymhall corymhall commented Jun 28, 2023

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
Copy link

gitpod-io bot commented Jun 28, 2023

@aws-cdk-automation aws-cdk-automation requested a review from a team June 28, 2023 11:42
@github-actions github-actions bot added the p2 label Jun 28, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jun 28, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

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.

@corymhall corymhall marked this pull request as draft June 28, 2023 11:45
@corymhall corymhall added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jun 29, 2023
@corymhall corymhall marked this pull request as ready for review June 29, 2023 14:53
@aws-cdk-automation aws-cdk-automation dismissed their stale review June 29, 2023 14:54

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

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

@aws-cdk-automation 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
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

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?

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

/**
* @see ArtifactMetadataEntryType.ACKNOWLEDGE
*/
export interface AcknowledgementMetadataEntry {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Mutations in the THEN part of a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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' } },
Copy link
Contributor

@rix0rrr rix0rrr Jun 30, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or honestly, a better message would probably be:

warning message here [warning1]

Comment on lines 120 to 123
if (entry.type === cxschema.ArtifactMetadataEntryType.ACKNOWLEDGE) {
const data = (entry.data as cxschema.AcknowledgementMetadataEntry);
acks.set(data.id, data.scopes);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 rix0rrr changed the title feat(core): acknowledge annotation warning messages feat(core): acknowledge warnings Jun 30, 2023
@aws-cdk-automation 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
@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 30, 2023

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.

Comment on lines 178 to 180
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@corymhall corymhall marked this pull request as draft July 6, 2023 10:07
@rix0rrr rix0rrr marked this pull request as ready for review August 18, 2023 13:32
@mergify
Copy link
Contributor

mergify bot commented Aug 23, 2023

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
Copy link
Contributor

mergify bot commented Aug 23, 2023

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
Copy link
Collaborator

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
Copy link
Contributor

mergify bot commented Aug 23, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants