Repository navigation
Conversation
dgandhi62
had a problem deploying
to
automation
October 25, 2026 18:39 — with
GitHub Actions
Failure
dgandhi62
had a problem deploying
to
automation
October 25, 2026 18:39 — with
GitHub Actions
Failure
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
Total lines changed 4023 is greater than 1000. Please consider breaking this PR down. |
dgandhi62
commented
Oct 27, 2026
Comment on lines
+223
to
+238
| librarySource.set(new RunnerLibraryNpmSource('aws-cdk-lib', args['framework-version']), '--framework-version'); | ||
| } | ||
|
|
||
| // Framework already installed in a given directory, no network install. | ||
| if (args['framework-path']) { | ||
| librarySource.set(new RunnerLibraryPreinstalledSource('aws-cdk-lib', args['framework-path']), '--framework-path'); | ||
|
|
||
| // Tests that set up a CDK app link these into the app alongside the framework, and | ||
| // never install them from the registry, so they must be installed there too. | ||
| for (const companion of FRAMEWORK_COMPANION_PACKAGES) { | ||
| readInstalledPackageJson(path.resolve(args['framework-path']), companion); | ||
| } | ||
| } | ||
|
|
||
| // Library source is either the given one, or 'latest' (nice and simple) | ||
| const librarySource: IRunnerSource<ITestLibrarySource> | ||
| = new RunnerLibraryNpmSource('aws-cdk-lib', args['framework-version'] ? args['framework-version'] : 'latest'); | ||
| if (!librarySource.isSet()) { |
Contributor
Author
There was a problem hiding this comment.
Default is to install the latest, as before
dgandhi62
commented
Oct 27, 2026
| toolkitSource.set(RunnerLibraryGlobalInstallSource.fromInstallRoot(toolkitLibPackage, args['toolkit-lib-path']), '--toolkit-lib-path'); | ||
| } | ||
|
|
||
| if (!toolkitSource.isSet()) { |
Contributor
Author
There was a problem hiding this comment.
Same default as before
dgandhi62
commented
Oct 27, 2026
| @@ -137,6 +165,8 @@ async function main() { | |||
| // Also, some of them depend on each other for convenience. | |||
| const cliSource = new UniqueOption<IRunnerSource<ITestCliSource>>('CLI version'); | |||
Contributor
Author
There was a problem hiding this comment.
ITestCliSource makes cdk runnable inside a test.
dgandhi62
commented
Oct 27, 2026
| // Also, some of them depend on each other for convenience. | ||
| const cliSource = new UniqueOption<IRunnerSource<ITestCliSource>>('CLI version'); | ||
| const cdkAssetsSource = new UniqueOption<IRunnerSource<ITestCliSource>>('cdk-assets version'); | ||
| const librarySource = new UniqueOption<IRunnerSource<ITestLibrarySource>>('Framework version'); |
Contributor
Author
There was a problem hiding this comment.
ITestLibrarySource defines the package that the test code loads
dgandhi62
commented
Oct 27, 2026
Contributor
Author
There was a problem hiding this comment.
Helper file that looks in the folder specified and answers - "is this package already on disk, and what version is it?"
…ization The test process reconstructs each package source from $SOURCES by looking up its class name in the CONSTRUCTORS list in subprocess.ts. The preinstalled sources (TestCliPreinstalledSource, TestLibraryPreinstalledSource) and TestLibraryGlobalInstallSource were never registered there, so any test using a withCdkApp-family fixture failed at TestFixture construction with 'Unrecognized constructor' once a --*-preinstalled flag was used. Register the three missing test-side source classes.
--framework-preinstalled only covered how the runner resolves aws-cdk-lib. Tests that set up a CDK app (including every bootstrap test) still ran 'npm install' for aws-cdk-lib and constructs in each test directory. Introduce a common IPackageInstaller interface behind installNpmPackages: - NpmPackageInstaller: the existing registry install, moved unchanged (REPO mode substitution, shared per-machine install, ECONNRESET retry). - PreinstalledPackageInstaller: links each requested package from the preinstalled location into the test's node_modules. It never falls back to the registry and fails listing every missing package. The installer is chosen from the framework source, so a run started with --framework-preinstalled always reuses preinstalled packages. constructs is installed next to aws-cdk-lib but has no flag of its own, so run-suite now checks up front that it is preinstalled when --framework-preinstalled is set. Preinstalled package resolution is consolidated into package-sources/preinstalled.ts, and the README lists what must be present to run the bootstrap tests preinstalled.
TestCliPreinstalledSource duplicated TestCliNpmSource: both only add a node_modules/.bin directory to the $PATH. Rename TestCliNpmSource to TestCliSource, since it is not tied to npm, and have the preinstalled CLI source serialize to it with the install root instead of a bin directory. This also fixes the bin directory for symlinked preinstalled packages. The old code derived it from the resolved (symlink-followed) package.json, so a package linked into tests/node_modules pointed the $PATH at a .bin next to the link target rather than the one next to the link. The install root is now found by searching upwards without following symlinks.
…ions A boolean preinstalled flag can't say where a package is installed, so a custom location would need a second option per component. Replace --cli-preinstalled, --cdk-assets-preinstalled, --framework-preinstalled and --toolkit-lib-preinstalled with --cli-path, --cdk-assets-path, --framework-path and --toolkit-lib-path, next to the existing --*-version options. Each takes the directory the package was installed into (the one containing its node_modules), so packages are no longer looked up from a fixed location under tests/. - --cli-path / --cdk-assets-path use TestCliSource with that directory, and fail up front if the package's executables are not in node_modules/.bin, so tests can't silently pick up another copy from the $PATH. - --toolkit-lib-path reuses RunnerLibraryGlobalInstallSource, skipping its npm install and linking the installed package instead. - --framework-path makes per-test setup link aws-cdk-lib and constructs from that directory. The installer is now chosen from the library source's install directory rather than an instanceof check.
The shared CDK app fixture (withSpecificCdkApp) always clones the app, installs aws-cdk-lib/constructs, and logs in to public ECR. A test that only bootstraps and inspects the stack needs none of these, yet still pays for them - and ECR login / the npm install both require network access. Add a FixtureSetupOptions object (cloneApp, installPackages, ecrLogin), all defaulting to true, threaded through withoutBootstrap/withCdkApp/ withSpecificFixture/withExtendedTimeoutFixture. Existing tests are unchanged. Apply it to the add-tags bootstrap test, which neither synthesizes nor deploys, as the first consumer.
… tests
16 bootstrap tests only run cdk bootstrap and inspect the result through
the AWS SDK. None of them pulls a public ECR image, so opt each one out of
the fixture's ECR login with setup: { ecrLogin: false }.
That login runs 'docker login public.ecr.aws', which needs Docker and
internet access, and always fails against a local AWS emulator: the
emulator issues the token, but the real registry rejects it.
The app clone and package install are left on: with no explicit
environment, 'cdk bootstrap' reads cdk.json and runs the app to find the
target environment. The 8 bootstrap tests that deploy a stack are
unchanged.
…d fromInstallRoot - subprocess.test.ts: every source the runner can produce survives the $SOURCES round trip into a test process. Guards against a source class missing from CONSTRUCTORS, which only fails once a test rebuilds it. - run-suite.test.ts: giving both a version and a path for the CLI, framework or toolkit lib fails, and --framework-path fails up front when constructs is not installed next to aws-cdk-lib. Runs the compiled script with npm pointed at an unreachable registry, so no case can install. - library-globalinstall-source.test.ts: fromInstallRoot links the installed package without running npm, removes only the link on dispose, and fails clearly when the package is missing.
…tead of a separate installer module Remove lib/package-installer.ts. installNpmPackages is back to the code on main, with one branch added at the top: when the framework source has an install directory (--framework-path), call linkPreinstalledPackages and return without touching the registry. linkPreinstalledPackages is the former PreinstalledPackageInstaller as a plain function, with unchanged behavior. Its tests move to link-preinstalled-packages.test.ts.
Trim the cli-preinstalled-source, library-globalinstall-source, run-suite and subprocess unit tests to the main success case and the most important failure each. Dropped cases were variants or already covered elsewhere.
…s-cdk-lib State in the README that constructs must be in the --framework-path directory and that npm 7+ installs it with aws-cdk-lib, and reword the comment on the up-front constructs check in run-suite.
Merge the cli-preinstalled-source and subprocess tests into package-sources.test.ts, with two tests: sources for packages installed in a given directory are used without installing and rebuild in a test process, and a CLI whose executables are missing from node_modules/.bin is rejected. Drop the run-suite and library-globalinstall-source tests. fromInstallRoot is exercised by the merged test, and the run-suite option checks fail with clear errors at startup.
This PR now only adds the --*-path options. Remove FixtureSetupOptions from with-cdk-app and the ecrLogin: false opt-outs from the 16 bootstrap tests; those move, unchanged in intent, to their own PR. Tests can still need network access for the public ECR login, as the README already notes.
… branch This branch carries both changes together again: the --*-path options and the fixture setup opt-outs (FixtureSetupOptions and ecrLogin: false in the 16 non-deploying bootstrap tests). The --*-path options alone are on integ-preinstalled-packages, and the opt-outs alone are on integ-fixture-setup-opt-outs. This reverts commit 438b209.
This branch had an error being deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
run-suitegets each test dependency from the npm registry, either by installing it or by looking up a version. That makes it impossible to run the suite in an isolated environment where every dependency has already been installed ahead of time. Even when the runner is pointed at existing packages, every test that sets up a CDK app still runs its ownnpm installforaws-cdk-libandconstructs.This PR adds a
--*-pathoption for each of the four test dependencies, next to the existing--*-versionoptions. Each takes the directory the package was installed into (the one containing itsnode_modules), and tests use that installation without reaching the registry.--cli-path=DIRaws-cdkDIR/node_modules/.binto the$PATH--cdk-assets-path=DIRcdk-assetsDIR/node_modules/.binto the$PATH--toolkit-lib-path=DIR@aws-cdk/toolkit-libnode_modules--framework-path=DIRaws-cdk-libaws-cdk-libandconstructsfromDIRinstead of runningnpm installA component takes either its version option or its path option, not both.
run-suitegets every test dependency from the npm registry, so the integration tests can't run in an isolated environment where the packages were installed ahead of time. The runner installs or looks up the CLI, cdk-assets, toolkit-lib, and the framework. On top of that, every test that sets up a CDK app runs its ownnpm installforaws-cdk-libandconstructs.This PR adds a
--*-pathoption for each of the four test dependencies, next to the existing--*-versionoptions. Each one takes the directory a package was installed into (the one containing itsnode_modules), and tests use that installation without contacting the registry. It also lets tests skip fixture setup steps they don't need, and the bootstrap tests that never pull a public image now skip the public ECR login.Usage
Install the packages into one directory ahead of time, then pass it to every option:
--cli-path=DIRaws-cdkDIR/node_modules/.binto the$PATH--cdk-assets-path=DIRcdk-assetsDIR/node_modules/.binto the$PATH--toolkit-lib-path=DIR@aws-cdk/toolkit-libDIR--framework-path=DIRaws-cdk-libaws-cdk-libandconstructsfromDIRinto each test app instead of runningnpm install--framework-path,constructsmust be installed in the same directory asaws-cdk-lib.AWS_REGIONSto several regions (for exampleus-east-1,us-east-2,us-west-2) to run them in parallel. With one region, the bootstrap suite runs one test at a time and takes over an hour.What the bootstrap tests need
Every bootstrap test needs AWS credentials, with permission to create the bootstrap stack's S3 bucket, ECR repository, IAM roles, and SSM parameter. Beyond that, they fall into three groups:
public.ecr.aws, for the ECR loginpublic.ecr.aws, for the ECR login and the base imageRun a single test with
-Fand part of its file name, for example-F cdk-bootstrap-add-tags.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license