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

(aws-cdk-lib/assertions): Simplify assertions on stack tags #27620

Closed
1 of 2 tasks
jamestelfer opened this issue Oct 20, 2023 · 3 comments · Fixed by #29247
Closed
1 of 2 tasks

(aws-cdk-lib/assertions): Simplify assertions on stack tags #27620

jamestelfer opened this issue Oct 20, 2023 · 3 comments · Fixed by #29247
Labels
@aws-cdk/assertions Related to the @aws-cdk/assertv2 package effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1

Comments

@jamestelfer
Copy link
Contributor

jamestelfer commented Oct 20, 2023

Describe the feature

Tags associated with a Cloudformation stack are recorded in the cloud assembly manifest. The existing Template and Annotations classes in aws-cdk-lib/assertions allow for simplified testing of some parts of the cloud assembly, but not the tags member.

It is possible to get the data from the manifest by interrogating the cloud assembly, but this is far less straightforward than the existing mechanisms.

Use Case

We have constructs libraries that ensure our stacks are tagged appropriately for a project. Currently tests for this behaviour are possible but not straightforward.

Proposed Solution

I suggest additions the API such that it would be possible to write assertions similar to:

// Tags aren't technically part of the template, though this kinda makes sense
Template.fromStack(foo).hasTags({
  "tag-name":"tag-value",
  "other-tag": Matcher.absent()
})

// OR - this is targeted carefully at this case
Tags.fromStack(foo).hasTags(Matcher.anyValue())

// OR - more generally, could be use for other manifest properties but I don't know what they would be.
StackProperties.fromStack(foo).hasTags(Matcher.anyValue())

Other Information

It is possible to write these tests outside of the CDK assertions library by interrogating the Cloud Assembly API. 4

Something like the following works:

test("foo", () => {
  const app = new TestApp()
  const stack = new TestStack(app, "name", {
    tags: { "tag-name": "tag-value" },
  })

  const stage = Stage.of(stack)
  if (!Stage.isStage(stage)) {
    throw new Error("unexpected: all stacks must be part of a Stage or an App")
  }

  const assembly = stage.synth()
  const tags = assembly.getStackArtifact(stack.artifactId).tags

  expect(tags).toMatchObject({
    "tag-name": "tag-value",
  })
})

There are some special cases that occur when looking up details in the assembly in both Template and Annotations classes. Respectively, handling nested stacks and forcing synthesis. I'm not sure which of these special cases should be handled in the tag case.

Template handling of nested stacks:

if (stack.nestedStackParent) {
// if this is a nested stack (it has a parent), then just read the template as a string
return JSON.parse(fs.readFileSync(path.join(assembly.directory, stack.templateFile)).toString('utf-8'));
}

Annotations forcing synthesis:

// to support incremental assertions (i.e. "expect(stack).toNotContainSomething(); doSomething(); expect(stack).toContainSomthing()")
const force = true;
const assembly = root.synth({ force });

I've started on a branch to feel out what an implementation would look like. If the Tags API is selected, the PR would be quite small by the looks.

Acknowledgements

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

CDK version used

2.102.0

Environment details (OS name and version, etc.)

N/A

@jamestelfer jamestelfer added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Oct 20, 2023
@github-actions github-actions bot added the @aws-cdk/assertions Related to the @aws-cdk/assertv2 package label Oct 20, 2023
@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Oct 20, 2023
@jamestelfer
Copy link
Contributor Author

jamestelfer commented Oct 21, 2023

My intent with this is to go ahead with a PR that introduces the Tags class. It's the simplest and clearest API that aligns to the underlying structure.

It makes some sense to have it on the template, as that may align with the mental model of the tags applying directly to the stack as the e do to other resources, but CloudFormation treats them differently so it's probably correct for the API to follow the actual model here.

WRT to the special cases listed above, I'm going to ignore both of them. I don't know that the nested stack issue affects tagging (happy to be corrected) and forcing re-synthesis like Annotations does is a surprising (and purely in my opinion undesirable) side-effect with a hidden impact on test performance.

@jamestelfer
Copy link
Contributor Author

jamestelfer commented Oct 21, 2023

I've raised a PR with a possible implementation. It's likely to need cleaning up, but I'll wait on feedback that it's in the ballpark before refining it carefully.

@jamestelfer jamestelfer changed the title (aws-cdk-lib/assertions): Simplify assertions on stack tags feat(aws-cdk-lib/assertions): Simplify assertions on stack tags Nov 13, 2023
@jamestelfer jamestelfer changed the title feat(aws-cdk-lib/assertions): Simplify assertions on stack tags (aws-cdk-lib/assertions): Simplify assertions on stack tags Nov 13, 2023
@scanlonp scanlonp added p1 and removed p2 labels Mar 22, 2024
@mergify mergify bot closed this as completed in #29247 Apr 10, 2024
mergify bot pushed a commit that referenced this issue Apr 10, 2024
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

```ts
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:

```ts
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:

```ts
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:

```ts
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:

```ts
class Tags {
  public static fromStack(stack: Stack) : Tags;
  public hasValues(props: any): void;
  public values():  { [key: string]: string };
}
```
and
```ts
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*
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/assertions Related to the @aws-cdk/assertv2 package effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1
Projects
None yet
3 participants