Skip to content

Conversation

@rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Nov 18, 2025

When using the toolkit-lib deployment engine, the selected region was not being passed to the CDK app.

In the old engine, we could set the AWS_REGION environment variable and the CDK CLI would respect that, but the toolkit lib was passing that environment variable directly to the CDK app which was not respecting that. Fix that by threading the region through as an explicit parameter and adding an override parameter to the AwsCliCompatible base credentials: we don't necessarily want to read this only from the INI files.

Additionally: add a validation to make sure that the CDK app respects the region we are requesting. CDK integ tests should either not pass an environment, or read $CDK_DEFAULT_REGION to determine the current region. It was too easy to just override the region with a hardcoded one, but that breaks the integ runner's attempts to isolate tests across regions. Guard against that by validating.

Also in this PR: disable the update workflow. I've lost 2 hours debugging why my deployment was failing with nonsenical values and the answer was: the checked-in snapshot was nonsensical, and the update workflow will first deploy the checked-in snapshot and only then deploy the current snapshot. Good idea, bad execution: if existing snapshots are not region-agnostic they are guaranteed to fail. We should probably resynthesize them from the previous git commit instead. Whatever it is, what we do right now doesn't work so I'm disabling it.

Fixes #


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

@rix0rrr rix0rrr requested a review from a team November 18, 2025 15:27
@github-actions github-actions bot added the p2 label Nov 18, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team November 18, 2025 15:28
@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.47%. Comparing base (29a6178) to head (207a22e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #949      +/-   ##
==========================================
- Coverage   84.50%   84.47%   -0.04%     
==========================================
  Files          71       71              
  Lines       10445    10445              
  Branches     1344     1344              
==========================================
- Hits         8827     8823       -4     
- Misses       1577     1581       +4     
  Partials       41       41              
Flag Coverage Δ
suite.unit 84.47% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

force: argv.force as boolean,
dryRun: argv['dry-run'] as boolean,
disableUpdateWorkflow: argv['disable-update-workflow'] as boolean,
disableUpdateWorkflow: true,
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

We can change the default to true, but not always set it to true so it can be used in the future.

private async validateRegion(asm: IReadableCloudAssembly) {
for (const stack of asm.cloudAssembly.stacksRecursively) {
if (stack.environment.region !== this.options.region && stack.environment.region !== UNKNOWN_REGION) {
throw new Error(`Stack ${stack.displayName} synthesizes for region ${stack.environment.region}, even though ${this.options.region} was requested. Please configure \`{ env: { region: process.env.CDK_DEFAULT_REGION, account: process.env.CDK_DEFAULT_ACCOUNT } }\`, or use no env at all. Do not hardcode a region or account.`);
Copy link
Member

Choose a reason for hiding this comment

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

Won't this fail for cross-region integ tests?

Copy link
Contributor Author

@rix0rrr rix0rrr Nov 19, 2025

Choose a reason for hiding this comment

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

Yes it will. In fact cross-region integ tests won't work properly with an external system that imposes a region at all.

What do you suggest instead?

Copy link
Member

Choose a reason for hiding this comment

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

For now, we can throw a warning instead of an error so we don't make integ test workflows fail unnecessarily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmkay. My personal life philosophy is: no one looks at warnings. So that will do literally diddly squat.

But okay, I'll make it a warning.

Copy link
Member

Choose a reason for hiding this comment

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

I share that philosophy too. Issue is that we currently have cross-region integ tests and this will block PR merges in aws-cdk whenever we update cross-region integ test snapshots.

@garysassano
Copy link

I was told this is not a bug just a couple of months ago: #624 (comment)

force: argv.force as boolean,
dryRun: argv['dry-run'] as boolean,
disableUpdateWorkflow: argv['disable-update-workflow'] as boolean,
disableUpdateWorkflow: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this a separate PR please, so we can have a separate discussion and a separate changelog entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split off to here: #951

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Nov 19, 2025

I was told this is not a bug just a couple of months ago: #624 (comment)

The issue you just linked is about doing:

cdk --region eu-west-1 bootstrap aws://11111111/us-east-1

What region should be bootstrapped?

This PR is about

integ-runner --parallel-regions ca-central-1,ap-southeast-1 <...>

When using the `toolkit-lib` deployment engine, the selected region
was not being passed to the CDK app.

In the old engine, we could set the `AWS_REGION` environment variable
and the CDK CLI would respect that, but the toolkit lib was passing that
environment variable directly to the CDK app which was not respecting
that. Fix that by threading the region through as an explicit parameter
and adding an override parameter to the `AwsCliCompatible` base
credentials: we don't necessarily want to read this only from the INI
files.

Additionally: add a validation to make sure that the CDK app respects
the region we are requesting. CDK integ tests should either not
pass an environment, or read `$CDK_DEFAULT_REGION` to determine the
current region. It was too easy to just override the region with a
hardcoded one, but that breaks the integ runner's attempts to isolate
tests across regions. Guard against that by validating.
@rix0rrr rix0rrr force-pushed the huijbers/integ-runner-region branch from 4f099e9 to a5226ac Compare November 19, 2025 09:24
@aws-cdk-automation aws-cdk-automation added this pull request to the merge queue Nov 19, 2025
Merged via the queue into main with commit f1db8df Nov 19, 2025
30 checks passed
@aws-cdk-automation aws-cdk-automation deleted the huijbers/integ-runner-region branch November 19, 2025 11:31
mergify bot pushed a commit to aws/aws-cdk that referenced this pull request Nov 21, 2025
### Reason for this change

Integ runner does not deploy to the correct regions that have been set to be bootstrapped.

### Description of changes

Updates the integ-runner version which fixes this bug. Fix is detailed in aws/aws-cdk-cli#949

As the new version changes the default CWD of integ tests, some integ tests had to be modified to use __dirname to lookup local assets files.

### Describe any new or updated permissions being added

No new permissions are added.

### Description of how you validated changes

Fix is confirmed with local fork run: https://github.com/Abogical/aws-cdk/actions/runs/19569552896/job/56039507543

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

6 participants