Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): CustomResourceProvider assets are staged in node_modules #20953

Merged
merged 15 commits into from
Jul 18, 2022
Merged

fix(core): CustomResourceProvider assets are staged in node_modules #20953

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8ccee3d
fix(core): CustomResourceProvider assets are staged in node_modules
otaviomacedo Jul 1, 2022
440050c
Updated pipeline integ tests
otaviomacedo Jul 4, 2022
30763a8
Merge branch 'main' into otaviom/temp-custom-resource-provider
otaviomacedo Jul 4, 2022
e1f5780
Updated yarn.lock
otaviomacedo Jul 4, 2022
72ff242
Updated hashes
otaviomacedo Jul 5, 2022
9b71c18
Merge branch 'main' into otaviom/temp-custom-resource-provider
otaviomacedo Jul 13, 2022
84e4673
Using Filesystem
otaviomacedo Jul 13, 2022
d664410
Copying the entire directory
otaviomacedo Jul 14, 2022
9a6186c
Updated pipeline integ tests
otaviomacedo Jul 15, 2022
718204a
Updated tests
otaviomacedo Jul 18, 2022
b7fd01f
Updated tests
otaviomacedo Jul 18, 2022
d2bbc53
Removed accidental changes
otaviomacedo Jul 18, 2022
26aee0b
Merge branch 'main' into otaviom/temp-custom-resource-provider
otaviomacedo Jul 18, 2022
22564af
Merge branch 'main' into otaviom/temp-custom-resource-provider
otaviomacedo Jul 18, 2022
0c69e63
Merge branch 'main' into otaviom/temp-custom-resource-provider
mergify[bot] Jul 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions packages/@aws-cdk/core/lib/custom-resource-provider/custom-resource-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as fs from 'fs';
import * as os from 'os';
import * as path from 'path';
import * as cxapi from '@aws-cdk/cx-api';
import { Construct } from 'constructs';
Expand Down Expand Up @@ -200,16 +201,17 @@ export class CustomResourceProvider extends Construct {

const stack = Stack.of(scope);

// copy the entry point to the code directory
fs.copyFileSync(ENTRYPOINT_NODEJS_SOURCE, path.join(props.codeDirectory, `${ENTRYPOINT_FILENAME}.js`));

// verify we have an index file there
if (!fs.existsSync(path.join(props.codeDirectory, 'index.js'))) {
throw new Error(`cannot find ${props.codeDirectory}/index.js`);
}

const stagingDirectory = fs.mkdtempSync(path.join(os.tmpdir(), 'cdk-custom-resource'));
Copy link
Contributor

Choose a reason for hiding this comment

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

using import { FileSystem } from '../fs';

Suggested change
const stagingDirectory = fs.mkdtempSync(path.join(os.tmpdir(), 'cdk-custom-resource'));
const stagingDirectory = FileSystem.mkdtemp('cdk-custom-resource');

Copy link
Contributor

Choose a reason for hiding this comment

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

It has the advantage of using the real temp dir even when it is a symlink like on OSx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks!

fs.copyFileSync(path.join(props.codeDirectory, 'index.js'), path.join(stagingDirectory, 'index.js'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously we were copying the ENTRYPOINT_FILENAME into the codeDirectory. Now we are copy the ENTRYPOINT_FILENAME and just the index.js file from the codeDirectory into a tmp directory. What happens if some custom resources have more than just an index.js file that needs to be copied?

For example https://github.com/aws/aws-cdk/tree/main/packages/%40aws-cdk/aws-iam/lib/oidc-provider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right! Done.

fs.copyFileSync(ENTRYPOINT_NODEJS_SOURCE, path.join(stagingDirectory, `${ENTRYPOINT_FILENAME}.js`));

const staging = new AssetStaging(this, 'Staging', {
sourcePath: props.codeDirectory,
sourcePath: stagingDirectory,
});

const assetFileName = staging.relativeStagedPath(stack);
Expand Down
11 changes: 8 additions & 3 deletions packages/@aws-cdk/core/test/custom-resource-provider/custom-resource-provider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,20 @@ describe('custom resource provider', () => {
});

// THEN
expect(fs.existsSync(path.join(TEST_HANDLER, '__entrypoint__.js'))).toEqual(true);
const cfn = toCloudFormation(stack);

// The asset hash constantly changes, so in order to not have to chase it, just look
// it up from the output.
const staging = stack.node.tryFindChild('Custom:MyResourceTypeCustomResourceProvider')?.node.tryFindChild('Staging') as AssetStaging;
const assetHash = staging.assetHash;
const sourcePath = staging.sourcePath;
const paramNames = Object.keys(cfn.Parameters);
const bucketParam = paramNames[0];
const keyParam = paramNames[1];
const hashParam = paramNames[2];

expect(fs.existsSync(path.join(sourcePath, '__entrypoint__.js'))).toEqual(true);

expect(cfn).toEqual({
Resources: {
CustomMyResourceTypeCustomResourceProviderRoleBD5E655F: {
Expand Down Expand Up @@ -139,9 +141,12 @@ describe('custom resource provider', () => {
// Then
const lambda = toCloudFormation(stack).Resources.CustomMyResourceTypeCustomResourceProviderHandler29FBDD2A;
expect(lambda).toHaveProperty('Metadata');
expect(lambda.Metadata).toEqual({
'aws:asset:path': `${__dirname}/mock-provider`,

expect(lambda.Metadata).toMatchObject({
'aws:asset:property': 'Code',

// The asset path should be a temporary folder prefixed with 'cdk-custom-resource'
'aws:asset:path': expect.stringMatching(/^.*\/cdk-custom-resource\w{6}\/?$/),
});

});
Expand Down