ci: add codebuild deployment integ runner PR workflow. by Abogical · Pull Request #35459 · aws/aws-cdk

Abogical · 2025-09-10T12:54:54Z

Reason for this change

Deploying integration tests has been time-consuming and expensive for both maintainers and contributors of aws-cdk. Snapshot update and deployment may take from 10 minutes to hours depending on the snapshots updates made for each pull request. This can make any changes to aws-cdk, no matter how simple they are, be needlessly time-consuming and error-prone.
AWS CDK's PR workflows does not validate that the snapshots of contributors are deployable. Although this is not common, snapshots updates can be made via a dry run, and there is a possibility that those snapshot updates won't be deployable. No PR Github action at the moment prevents this scenario.

Description of changes

This adds a PR action which will use our Internal environment to test snapshot updates by attempting to deploy them. This uses the same service used by aws-cdk-cli (called Atmosphere) to deploy only the snapshot updates for the PR. This PR will need manual approval by a maintainer to run for the PR to work.

Mergify rules will also be updated such that if a request to run the integ tests are made, then this workflow should pass successfully in order for the PR to be merged.

Description of how you validated changes

This workflow was validated with my fork of aws-cdk. See this example run: https://github.com/Abogical/aws-cdk/actions/runs/17458740897/job/49578262885

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

Abogical · 2025-09-11T14:35:44Z

Waiting for the backend infrastructure to be setup before this is merged. Hence the do-not-merge label.

Abogical · 2025-09-11T16:45:38Z

Backend infrastructure is setup. Removing do-not-merge label.

.github/workflows/codebuild-pr-deployment-integ.yml

gasolima · 2025-09-12T12:08:25Z

tools/@aws-cdk/deployment-integ/lib/integ-run.ts

+
+export const runInteg = async (paths: string[], allocation: Allocation) => {
+  if (paths.length == 0) {
+    console.log('No snapshots changes were made, skipping deployment integ test.');


Shouldn't throw an error here? cause if we run the workflow that mean someone know that there's integration tests that need to be deployed

That would not be considered an error of the PR or the workflow. We may notify the maintainers of this via a job summary stating that no snapshots has been deployed:

Related: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands#adding-a-job-summary

gasolima · 2025-09-12T12:09:19Z

tools/@aws-cdk/deployment-integ/lib/integ-run.ts

+  const spawnProcess = spawn('yarn', ['integ-runner', '--directory', 'packages', '--force', ...paths], {
+    stdio: ['ignore', 'inherit', 'inherit'],
+    env: {
+      ...process.env,


What other envs needed?

This just allows passing env variables to be passed on to the integ runner so that we can extend the workflow easily in the future.

I'm worried about this from security perspective, not sure what can process.env contain, don't want to run a command with every thing in our environments.

Can we have here something like

env: { AWS_ACCESS_KEY_ID: props.awsAccessKeyId ?? process.env.AWS_ACCESS_KEY_ID , etc... }

gasolima · 2025-09-12T12:44:14Z

tools/@aws-cdk/deployment-integ/lib/git.ts

+import { spawn } from 'child_process';
+
+export const gitDiff = async (): Promise<string> => {
+  const spawnProcess = spawn('git', ['diff', process.env.BASE_COMMIT ?? 'main', process.env.HEAD_COMMIT ?? 'HEAD', '--name-only'], {


When are we using this default values(main & HEAD)?

gasolima · 2025-09-12T12:50:07Z

.github/workflows/codebuild-pr-deployment-integ.yml

+            ~/.s3buildcache
+          key: s3buildcache-${{ runner.os }}
+
+      - name: Configure system settings


What kind of configuration we're setting here

This is just increasing memory so that workflow does not fail due to out of memory error.

can we add this in the naming? If it's not possible as comment

A comment has been added.

gasolima · 2025-09-12T12:56:05Z

.github/workflows/codebuild-pr-deployment-integ.yml

+          sudo sysctl -w vm.max_map_count=2251954
+
+      - name: Build
+        run: /bin/bash ./build.sh --ci --skip-test --skip-prereqs --skip-compat


I'm expecting to put this in a package.json file, then here only have something like yarn build-github

gasolima · 2025-09-12T12:56:41Z

.github/workflows/codebuild-pr-deployment-integ.yml

+        run: /bin/bash ./build.sh --ci --skip-test --skip-prereqs --skip-compat
+
+      - name: Run deployment integration tests
+        run: node tools/@aws-cdk/deployment-integ/bin/deployment-integ.js


same comment: I'm expecting to put this in a package.json file, then here only have something like yarn deploy-integration-tests

gasolima · 2025-09-12T13:00:12Z

tools/@aws-cdk/deployment-integ/README.md

+
+```bash
+# Install dependencies
+npm install


I believe we shouldn't use npm, right?

gasolima · 2025-09-12T13:15:41Z

tools/@aws-cdk/deployment-integ/lib/main.ts

+    val => val.match(/^.*integ\.[^/]*\.js/)?.[0] || null,
+  ).filter(val => val !== null))];
+
+export const main = async ({ endpoint, pool }: {endpoint: string; pool: string}) => {


Can we have more descriptive name than main?

gasolima · 2025-09-12T13:21:12Z

tools/@aws-cdk/deployment-integ/lib/integ-run.ts

+import { Allocation } from '@cdklabs/cdk-atmosphere-client';
+import { spawn } from 'child_process';
+
+export const runInteg = async (paths: string[], allocation: Allocation) => {


I would like from this function to not have anything related to Atmosphere so people can run the same script locally using their credentials. Does that make sense?

gasolima · 2025-09-23T13:12:29Z

.github/workflows/integration-test-deployment.yml

+        working-directory: base
+        run: yarn --cwd tools/@aws-cdk/integration-test-deployment build
+
+      - name: Install dependencies for framework-integ (HEAD)


framework-integ is integration tests, right? can we make call it integration test also for line 103.

gasolima · 2025-09-23T13:14:23Z

tools/@aws-cdk/integration-test-deployment/lib/integration-test-runner.ts

+
+  try {
+    const env = {
+      ...process.env,


can we avoid doing this? you can extract whatever variables you want manually

gasolima · 2025-09-23T13:15:34Z

tools/@aws-cdk/integration-test-deployment/lib/integration-test-runner.ts

+  }
+};
+
+export const runInteg = async (env: NodeJS.ProcessEnv) => {


isn't it better to make it deployIntegrationTests?

Also will add a parameter to run deploy all integt est as per our last conversation.

gasolima · 2025-09-23T13:17:16Z

tools/@aws-cdk/integration-test-deployment/test/git-diff-snapshot.txt

can we have here max 5 files? just for readability and easy spotting problems

I have removed some, but reducing more defeats the purpose of the test.

tools/@aws-cdk/integration-test-deployment/.eslintrc.js

gasolima · 2025-09-23T13:20:02Z

tools/@aws-cdk/integration-test-deployment/README.md

Can you differentiate here between calling Atmosphere vs ur setup? can u state how each one can be done.

Okay, I can state how our setup can be used but, atmosphere cannot be used by anyone since it requires permission. For now only the pr build role will have the permission to assume role for atmosphere..

tmosphere cannot be used by anyone since it requires permission.

mention that in the doc

gasolima · 2025-09-23T13:28:58Z

.github/workflows/integration-test-deployment.yml

+jobs:
+  integration_test_deployment:
+    runs-on: codebuild-aws-cdk-github-actions-deployment-integ-runner-${{ github.run_id }}-${{ github.run_attempt }}
+    environment: deployment-integ-test


Can you add here warning to never change this unless rediscussing the topic with Appsec?

what do you mean?

A code comment saying that never change this unless rediscussing the topic with Appsec, i believe this line if we removed will remove the approval step, which we were always having this assumption in our talks, and whenever we want to change this assumption we need to go back and see the different threats.

gasolima · 2025-09-23T15:47:54Z

.github/workflows/integration-test-deployment.yml

In general now it's way more clear! Good job 👏

.github/workflows/integration-test-deployment.yml

gasolima · 2025-09-24T08:09:05Z

.github/workflows/integration-test-deployment.yml

+on:
+  workflow_dispatch: {}
+  merge_group: {}
+  pull_request_target:


FYI: Since I was the one who suggested using pull_request_target instead of pull_request, I wanted to clarify the reasoning — some of the behavior is a bit unintuitive.

pull_request_target runs with write permissions and access to repository secrets, whereas pull_request does not have these for external forks.

However, the key difference is where the workflow runs from.

With pull_request, the workflow is executed from the PR’s commit (HEAD). That means a malicious contributor could modify the workflow in their fork, and it would execute automatically as soon as they open a PR — before we ever review or approve it.

With pull_request_target, the workflow runs from the base branch of our repository (e.g., main). This ensures that if someone tries to sneak malicious code into a workflow file in their PR, it won’t run until we explicitly approve and merge it.

Note: “write permission and secrets access” only apply to GitHub-managed credentials. Our main concern here is the OIDC role — we want to prevent anyone from being able to exploit that role. GitHub doesn’t enforce that restriction for us.

So, while pull_request_target technically grants more powerful permissions if misused, it is still the safer option. It prevents unreviewed, potentially malicious workflows from running automatically, which is a bigger risk in our case.

gasolima · 2025-09-26T15:15:37Z

tools/@aws-cdk/integration-test-deployment/lib/integration-test-runner.ts

+};
+
+export const deployIntegrationTests = async (env: NodeJS.ProcessEnv) => {
+  const changedSnapshotPaths = await getChangedSnapshots();


Can we have a followup PR to make this function possible to deploy all the tests by specific parameter send?

acornea · 2025-09-30T08:25:48Z

LGTM

mergify · 2025-10-07T09:18:43Z

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 · 2025-10-07T09:59:47Z

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

github-actions · 2025-10-07T10:00:02Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

ci: add codebuild deployment integ runner PR workflow.

0bc10bb

aws-cdk-automation requested a review from a team September 10, 2025 12:55

github-actions bot added p2 admired-contributor [Pilot] contributed between 13-24 PRs to the CDK labels Sep 10, 2025

mergify bot added the contribution/core This is a PR that came from AWS. label Sep 10, 2025

add mergify rules

8d29f9b

Abogical added the pr/do-not-merge This PR should not be merged at this time. label Sep 11, 2025

Abogical marked this pull request as ready for review September 11, 2025 14:35

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 11, 2025

Abogical removed the pr/do-not-merge This PR should not be merged at this time. label Sep 11, 2025

mrgrain reviewed Sep 11, 2025

View reviewed changes

.github/workflows/codebuild-pr-deployment-integ.yml Outdated Show resolved Hide resolved

gasolima added the pr/do-not-merge This PR should not be merged at this time. label Sep 11, 2025

gasolima reviewed Sep 12, 2025

View reviewed changes

.github/workflows/codebuild-pr-deployment-integ.yml Outdated Show resolved Hide resolved

gasolima reviewed Sep 12, 2025

View reviewed changes

.github/workflows/codebuild-pr-deployment-integ.yml Outdated Show resolved Hide resolved

gasolima reviewed Sep 12, 2025

View reviewed changes

.github/workflows/codebuild-pr-deployment-integ.yml Outdated Show resolved Hide resolved

Abogical added 2 commits September 12, 2025 13:32

delete git log line and add mask to github action

47e56d7

Clarify comment in github action

1b6b23d

Abogical requested review from gasolima and mrgrain September 12, 2025 11:40

gasolima reviewed Sep 12, 2025

View reviewed changes

gasolima reviewed Sep 23, 2025

View reviewed changes

tools/@aws-cdk/integration-test-deployment/.eslintrc.js Outdated Show resolved Hide resolved

gasolima reviewed Sep 23, 2025

View reviewed changes

abidhasan-aws added 2 commits September 23, 2025 16:38

resolve comments

9cbfff5

update snapshot

52d254c

gasolima reviewed Sep 23, 2025

View reviewed changes

.github/workflows/integration-test-deployment.yml

Copy link
Copy Markdown

Contributor

gasolima Sep 23, 2025

Choose a reason for hiding this comment

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

In general now it's way more clear! Good job 👏

gasolima reviewed Sep 24, 2025

View reviewed changes

.github/workflows/integration-test-deployment.yml Outdated Show resolved Hide resolved

gasolima reviewed Sep 24, 2025

View reviewed changes

.github/workflows/integration-test-deployment.yml Outdated Show resolved Hide resolved

gasolima reviewed Sep 24, 2025

View reviewed changes

abidhasan-aws added 2 commits September 25, 2025 13:37

resolve comments

98eba37

update node version to lts

8b8e272

gasolima reviewed Sep 26, 2025

View reviewed changes

gasolima approved these changes Sep 26, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 26, 2025

github-actions bot mentioned this pull request Oct 1, 2025

Monthly PR metrics report #35642

Open

gasolima removed the pr/do-not-merge This PR should not be merged at this time. label Oct 7, 2025

mergify bot added the queued label Oct 7, 2025

Merge branch 'main' into deployment-integ-workflow-squashed

4a70566

mergify bot merged commit 919b14b into main Oct 7, 2025
19 checks passed

mergify bot removed the queued label Oct 7, 2025

mergify bot deleted the deployment-integ-workflow-squashed branch October 7, 2025 09:59

github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2025

Conversation

Abogical commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

Abogical commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Abogical commented Sep 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abidhasan-aws Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Abogical commented Sep 10, 2025 •

edited

Loading

Abogical commented Sep 11, 2025 •

edited

Loading

abidhasan-aws Sep 23, 2025 •

edited

Loading