Skip to content

Commit

Permalink
Merge branch 'cdklabs:main' into rds-query
Browse files Browse the repository at this point in the history
  • Loading branch information
pcheungamz authored Nov 26, 2024
2 parents c949cf6 + a59daa2 commit 0d7ab1b
Show file tree
Hide file tree
Showing 5 changed files with 224 additions and 109 deletions.
27 changes: 24 additions & 3 deletions lib/manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,17 @@ export class Manifest {
* @param filePath - path to the manifest file.
*/
public static loadIntegManifest(filePath: string): integ.IntegManifest {
return this.loadManifest(filePath, INTEG_SCHEMA);
const manifest = this.loadManifest(filePath, INTEG_SCHEMA);

// Adding typing to `validate()` led to `loadManifest()` to properly infer
// its return type, which indicated that the return type of this
// function may be a lie. I could change the schema to make `testCases`
// optional, but that will bump the major version of this package and I
// don't want to do that. So instead, just make sure `testCases` is always there.
return {
...manifest,
testCases: (manifest as any).testCases ?? [],
};
}

/**
Expand All @@ -147,7 +157,11 @@ export class Manifest {
return this.loadAssemblyManifest(filePath);
}

private static validate(manifest: any, schema: jsonschema.Schema, options?: LoadManifestOptions) {
private static validate(
manifest: any,
schema: jsonschema.Schema,
options?: LoadManifestOptions
): asserts manifest is assembly.AssemblyManifest {
function parseVersion(version: string) {
const ver = semver.valid(version);
if (!ver) {
Expand All @@ -161,10 +175,17 @@ export class Manifest {

// first validate the version should be accepted. all versions within the same minor version are fine
if (maxSupported < semver.major(actual) && !options?.skipVersionCheck) {
// If we have a more specific error to throw than the generic one below, make sure to add that info.
const cliVersion = (manifest as assembly.AssemblyManifest).minimumCliVersion;
let cliWarning = '';
if (cliVersion) {
cliWarning = `. You need at least CLI version ${cliVersion} to read this manifest.`;
}

// we use a well known error prefix so that the CLI can identify this specific error
// and print some more context to the user.
throw new Error(
`${VERSION_MISMATCH}: Maximum schema version supported is ${maxSupported}.x.x, but found ${actual}`
`${VERSION_MISMATCH}: Maximum schema version supported is ${maxSupported}.x.x, but found ${actual}${cliWarning}`
);
}

Expand Down
6 changes: 3 additions & 3 deletions package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions test/fixtures/high-version-with-cli/manifest.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"version": "99.99.99",
"minimumCliVersion": "minimumCliVersion",
}
6 changes: 6 additions & 0 deletions test/manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,12 @@ test('manifest load fails on higher major version', () => {
);
});

test('load error includes CLI error if available', () => {
expect(() => Manifest.loadAssemblyManifest(fixture('high-version-with-cli'))).toThrow(
/minimumCliVersion/
);
});

// once we start introducing minor version bumps that are considered
// non breaking, this test can be removed.
test('manifest load succeeds on higher minor version', () => {
Expand Down
Loading

0 comments on commit 0d7ab1b

Please sign in to comment.