Skip to content

Conversation

@rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Jun 27, 2025

The integ-runner uses synthFast to produce the assembly that will be used to compare the snapshot to, to determine if there is "a change".

However, when a snapshot has changed and a test is run, there is a fork:

  • In dry-run mode: synthFast will be used to produce the updated snapshot.
  • In real-mode, the result of this.deploy() will be the updated snapshot (but also: the next time it will be compared to the result of synthFast).

We are running into a situation where the results of synthFast and this.deploy are different, and snapshot validation fails.

In this change, we always generate the snapshot using synthFast, and ignore the template produced by this.deploy(). This makes sure that the templates will at least compile equal.

I'm not sure why this ever worked this way, because the template that is deployed can be specialized to an account and region, but the snapshot can never be, so we need to investigate a little more.


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

The integ-runner uses `synthFast` to produce the assembly that will
be used to compare the snapshot to, to determine if there is "a change".

However, when a snapshot has changed and a test is run, there is a fork:

- In dry-run mode: `synthFast` will be used to produce the updated
  snapshot.
- In real-mode, the result of `this.deploy()` will be the updated
  snapshot (but also: the next time it will be compared to the result
  of `synthFast`).

We are running into a situation where the results of `synthFast` and
`this.deploy` are different, and snapshot validation fails.

In this change, we always generate the snapshot using `synthFast`, and
ignore the template produced by `this.deploy()`. This makes sure
that the templates will at least compile equal.

I'm not sure why this ever worked this way, because the template
that is deployed can be specialized to an account and region, but
the snapshot can never be, so we need to investigate a little more.
@rix0rrr rix0rrr requested a review from a team June 27, 2025 12:20
@rix0rrr rix0rrr temporarily deployed to integ-approval June 27, 2025 12:20 — with GitHub Actions Inactive
@github-actions github-actions bot added the p2 label Jun 27, 2025
@rix0rrr rix0rrr self-assigned this Jun 27, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team June 27, 2025 12:20
@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jun 27, 2025

Okay the actual issue we had related to the flag @aws-cdk/aws-lambda:createNewPoliciesWithAddToRolePolicy being different between the 2 modes.

Don't know what that was about, but I still feel the snapshot-that-gets-commited and the snapshot-that-gets-compared should use the same code paths.

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jun 30, 2025

To close this out:

The source of this bug was that during deploy, we would invoke the CLI with cdk-cli-wrapper; it would pass context via the command-line, and in the command-line all context values are strings.

We would pass the boolean false as context, which would get implicitly cast to "false", which would evaluate to true. This causes flags to be different between "real" synthesis and "deployment" synthesis.

This behavior will be fixed when we make the new toolkit-lib-engine the default, because that engine does not suffer from the same type confusion. In the mean time, I think the proposed code refactoring in this PR still makes sense.

@aws-cdk-automation aws-cdk-automation added this pull request to the merge queue Jun 30, 2025
Merged via the queue into main with commit 7cac5d2 Jun 30, 2025
39 of 40 checks passed
@aws-cdk-automation aws-cdk-automation deleted the huijbers/integ-runner branch June 30, 2025 18:39
rix0rrr added a commit that referenced this pull request Jul 3, 2025
…okups: false`

In #666 we used `synthFast()` to
always generate a snapshot in the same was as was used for validating
(which was also using `synthFast()`).

The difference being, that the context we load depends on the
`enableLookups: false|true` flag that's passed to `new IntegTest()` in
the test case itself. So when writing the snapshot we have to take that
same field into account.
github-merge-queue bot pushed a commit that referenced this pull request Jul 3, 2025
…okups: false` (#685)

In #666 we used `synthFast()` to
always generate a snapshot in the same was as was used for validating
(which was also using `synthFast()`).

The difference being, that the context we load depends on the
`enableLookups: false|true` flag that's passed to `new IntegTest()` in
the test case itself. So when writing the snapshot we have to take that
same field into account.

---
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants