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(assertions): add stack tagging assertions #29247

Merged
merged 14 commits into from
Apr 10, 2024
Merged

feat(assertions): add stack tagging assertions #29247

merged 14 commits into from
Apr 10, 2024

Conversation

jamestelfer
Copy link
Contributor

Adds a Tag class to the assertions library that permits assertions against tags on synthesized CDK stacks.

Tags on AWS resources can be checked via assertions with the Template class, but since stack tags only appear on the cloud assembly manifest as a sibling of the template, a separate assertion mechanism is required.

Note

Previously submitted as #27633, which was closed automatically while waiting for review.

This PR is complete to the best of my knowledge, needing only an exemption from integration tests. As far as I can tell, this area of the library has no integration tests directly associated.

API

class Tags {
  public static fromStack(stack: Stack) : Tags;
  public hasValues(props: any): void;
  public all(): { [key: string]: string };
}

Usage

This new class permits tests of the form:

import { App, Stack } from 'aws-cdk-lib';
import { Tags } from 'aws-cdk-lib/assertions';

const app = new App();
const stack = new Stack(app, 'MyStack', {
  tags: {
    'tag-name': 'tag-value'
  }
});

const tags = Tags.fromStack(stack);

// using a default 'objectLike' Matcher
tags.hasValues({
  'tag-name': 'tag-value'
});

// or another Matcher
tags.hasValues({
  'tag-name': Match.anyValue()
});

You can also get the set of tags to test them in other ways:

tags.all()

Issues

No tags case

One might expect that the case where no tags are present would match undefined or null, but since the Cloud Assembly API defaults tags to {} when none are present, this isn't possible. It's also not practical to directly test the artifact.manifest.properties.tags value directly, as there is a legacy case that the API handles. This means that the artifact.tags property is the best value to check against.

The tests for this PR show that matching with Match.absent() will fail when there are no tags, but testing against the empty object will succeed.

I think that this behaviour (defaulting to empty) will be OK, but potentially require a callout on the assertion method.

API method naming

The current suggested API went through some evolution, starting with:

class Tags {
  public static fromStack(stack: Stack) : Tags;
  public hasTags(props: any): void;
  public getTags():  { [key: string]: string };
}

But this stuttered, and getTags() wasn't compatible with Java.

I considered:

class Tags {
  public static fromStack(stack: Stack) : Tags;
  public hasValues(props: any): void;
  public values():  { [key: string]: string };
}

and

class Tags {
  public static fromStack(stack: Stack) : Tags;
  public has(props: any): void;
  public all():  { [key: string]: string };
}

... before settling on a mix of the two. I think the current iteration fits with the rest of the assertions API and makes sense by itself, but very open to changes.

Closes #27620.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Feb 25, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team February 25, 2024 05:31
@jamestelfer
Copy link
Contributor Author

Exemption Request

As far as I can tell, the assertions library does not have integration tests, but I'm happy to be corrected.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Feb 25, 2024
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.

@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 Feb 25, 2024
Adds a Tag class to the assertions library that permits assertions
against tags on synthesized CDK stacks.
@jamestelfer jamestelfer changed the title feat(assertions): add stack tagging assertions feat(assertions): add stack tagging assertions [exemption requested] Mar 12, 2024
@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@scanlonp
Copy link
Contributor

Hey @jamestelfer, I'm taking a look. This functionality looks terrific!

I think we can write some integ tests, but probably in @aws-cdk-testing/framework-integ. I need to check out how the IntegTest.assertions work, but if they pull from this module, it would be great to use your new functionality! We can also highlight this in our integ testing guide if that's the case.

@scanlonp
Copy link
Contributor

@Mergifyio update

Copy link
Contributor

mergify bot commented Mar 21, 2024

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/repo-metrics-monthly.yml without workflows permission

@scanlonp
Copy link
Contributor

@jamestelfer turns out IntegTest.assertions are completely separate from the assertions module, so we don't need to worry about @aws-cdk-testing at all. I do not think this change needs integ testing.

@scanlonp scanlonp added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Mar 21, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review March 21, 2024 17:52

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

@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 Mar 21, 2024
@TheRealAmazonKendra TheRealAmazonKendra changed the title feat(assertions): add stack tagging assertions [exemption requested] feat(assertions): add stack tagging assertions Mar 22, 2024
Copy link
Contributor

@scanlonp scanlonp left a comment

Choose a reason for hiding this comment

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

@jamestelfer, thanks so much for this contribution! I really appreciate your detailed thought process in the PR description, and I think you settled on the best API.

Also thanks for bringing up testing for no tags. I agree its a little odd that Match.absent() does not work well. I have a couple ideas on getting around this.

  1. Is there a way to see what type of Match statement is passed into hasValues()? If so could you check for Match.absent and substitute Match.objectEquals({})?
  2. We could add a Tags.none() or Tags.hasNone() method that gives the result of Tags.hasValues(Match.objectEquals({})).

I would lean towards option 2 with hasNone(), but let me know what you think.

Last comment, I think it would be good to add some unit tests where there are multiple tags on the stack. That way we can see how hasValues() and all() interact with more than just the one key value pair.

packages/aws-cdk-lib/assertions/test/tags.test.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/assertions/test/tags.test.ts Outdated Show resolved Hide resolved
@scanlonp scanlonp added p1 and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. p2 labels Mar 22, 2024
Review feedback

Co-authored-by: Parker Scanlon <[email protected]>
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Mar 27, 2024
@jamestelfer
Copy link
Contributor Author

jamestelfer commented Mar 27, 2024

@scanlonp thanks for your thoughtful review and commentary!

I'm going to make the following changes as a result:

  1. I've kind of done both of your suggestions. There is now a hasNone() method as you suggested, and hasValue(Match.absent()) now throws an error directing the user to use hasNone(). I felt that an explanatory error instead of magically replacing the matcher would keep the API transparent about what's happening internally. Happy to be wrong on this if you'd like it different.
  2. I'll add some further tests, no probs!

Convenience method over `hasValues(Match.exact({}))`. Required as `Match.absent()` (the natural choice) will not act as the user expects.
… with the `absent` matcher

Since the value is defaulted to `{}`, the absent matcher will not work as expected.

We throw an error here instead of changing the Matcher under the covers. This ensures the API is transparent about the limitation, rather than covering it up.
@jamestelfer
Copy link
Contributor Author

😞 Rosetta doesn't like my doco updates. I'll come back around on this after easter.

Improvements to allow Rosetta to test the documentation.
Bring doc up to scratch, removing copy-pasta and properly explaining usage intent.
@jamestelfer
Copy link
Contributor Author

I've updated the documentation with some better examples, and included some tests with more keys and more complicated matching.

I did add a simple check on the matcher so there was a better error message to API consumers. I figured it was better to explicitly fail with a good message than switch the matcher transparently.

It's ready for another look when you're ready @scanlonp!

Copy link
Contributor

@scanlonp scanlonp left a comment

Choose a reason for hiding this comment

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

Looks terrific. Tests look great, and the documentation is fantastic.

I like how you handled the hasNone() and the transparent failing of Match.absent().

Again, appreciate the contribution! Excited for this to get used!

@scanlonp scanlonp self-assigned this Apr 9, 2024
Copy link
Contributor

mergify bot commented Apr 9, 2024

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 aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 9, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 72f189d into aws:main Apr 10, 2024
12 checks passed
Copy link
Contributor

mergify bot commented Apr 10, 2024

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
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 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.

(aws-cdk-lib/assertions): Simplify assertions on stack tags
3 participants