Skip to content

Conversation

@lachriz-aws
Copy link
Contributor

Thanks for creating this construct - it makes working with .NET Lambda functions in CDK a much better experience!

Out of curiosity I have been reading through the code base to learn more about how this construct has been implemented, and I noticed a minor optimization that can be done.

The construct was already (implicitly) using AUTO_DISCOVER as bundling output type, which according to the docs means:

If the bundling output directory contains a single archive file (zip or jar) it will be used as the bundle output as-is. Otherwise, all the files in the bundling output directory will be zipped.

This also means that if no hooks are registered, then there is a such no need to go through the work of unzipping the produced package.zip file, just for the CDK to zip it again.

This PR introduces a simple check, to see if hooks are registered, and if not, the produced command is simply just the dotnetPackageCommand which spits out a package.zip, which, due to the use of AUTO_DISCOVER as bundling output type, is then picked up and use as code asset by the CDK.

On top of this, while the bundling output type in CDK does default to AUTO_DISCOVER, I have now configured it in code, just to make it explicit that the this "auto mode" is in fact used as part of the construct's logic.

@lachriz-aws lachriz-aws changed the title Skip invocation of hooks and unzip / delete commands if no hooks are registered feat: Skip invocation of hooks and unzip / delete commands if no hooks are registered Aug 30, 2024
Copy link
Contributor

@vlesierse vlesierse left a comment

Choose a reason for hiding this comment

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

Great optimization as it removes the need to unpack the package output. Is the unzipCommand and deleteCommand still needed in this case and can we remove this as well?

@vlesierse
Copy link
Contributor

vlesierse commented Sep 5, 2024

This PR might be better to be marked as fix, as this doesn't introduce a feature but optimizes the bundling process.

Edit: Thinking about it, you want to make changes with the hook on the unpacked files and don't want to unzip them every time explicitly.

@lachriz-aws
Copy link
Contributor Author

Great optimization as it removes the need to unpack the package output. Is the unzipCommand and deleteCommand still needed in this case and can we remove this as well?

Edit: Thinking about it, you want to make changes with the hook on the unpacked files and don't want to unzip them every time explicitly.

Exactly, my first thought was the same - to remove the unzipCommand and the deleteCommand - however, as you also point out, one might want to use the hooks to make changes to the unpacked files and hence get everything re-packed once done (which is why I opted for leaving the commands in place).

This PR might be better to be marked as fix, as this doesn't introduce a feature but optimizes the bundling process.

Regarding feat: vs fix:, I see your point - let me update that right away.

@lachriz-aws lachriz-aws changed the title feat: Skip invocation of hooks and unzip / delete commands if no hooks are registered fix: Skip invocation of hooks and unzip / delete commands if no hooks are registered Sep 5, 2024
@aws-cdk-automation aws-cdk-automation added this pull request to the merge queue Sep 5, 2024
Merged via the queue into cdklabs:main with commit a9aaedc Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants