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.

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

feat(lambda-nodejs): custom asset hash #16412

Merged
merged 9 commits into from
Nov 8, 2021

Conversation

jogold
Copy link
Contributor

@jogold jogold commented Sep 8, 2021

By default the asset hash for a NodejsFunction is based on the
bundling output, the result of the esbuild bundling.

Add a assetHash prop to customize the asset hash and allow users to
implement their own logic. This can be useful if users want to invalidate
the asset based on specific (input) file contents or logic.

Closes #16157


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

Add a `assetHash` prop to customize the asset hash.

Closes aws#16157
@gitpod-io
Copy link

gitpod-io bot commented Sep 8, 2021

@jogold jogold changed the title feat(lamdba-nodejs): custom asset hash feat(lambda-nodejs): custom asset hash Sep 28, 2021
@jogold
Copy link
Contributor Author

jogold commented Sep 30, 2021

@nija-at can you check this? thx!

@nija-at nija-at self-assigned this Oct 4, 2021
@nija-at
Copy link
Contributor

nija-at commented Oct 4, 2021

@jogold - can you update the PR description with more details on what is going on here and why this is needed?

I couldn't fully understand it from the linked issue.

@jogold
Copy link
Contributor Author

jogold commented Oct 4, 2021

@jogold - can you update the PR description with more details on what is going on here and why this is needed?

I couldn't fully understand it from the linked issue.

updated

@nija-at
Copy link
Contributor

nija-at commented Oct 5, 2021

I'm still not following the motivation.

What is a use case for this? Why would we want to provide such an option?
Why is updating our current hash calculation not enough?

It feels that this is going to be used incorrectly more often than correctly, and thereby users are going to shoot themselves in the foot.

@jogold
Copy link
Contributor Author

jogold commented Oct 5, 2021

What is a use case for this? Why would we want to provide such an option?

@oyed can you detail your use case here?

One use case I can think of: you don't want the hash to change if the way esbuild bundles changes, e.g. for the same input files and dependencies the content of the resulting index.js is different but functionally remains the same. This can happen after an update of esbuild for example.

Identifying the specific input files of a NodejsFunction is almost impossible to achieve in the construct itself. The idea of the NodejsFunction is to allow users to mix infra and runtime code, so we don't know what is the "source".

It feels that this is going to be used incorrectly more often than correctly, and thereby users are going to shoot themselves in the foot.

We're just exposing an asset option from core which allows to do this. aws-lambda-go and aws-lambda-python are already exposing this.

@nija-at nija-at added effort/small Small work item – less than a day of effort p2 labels Oct 29, 2021
@nija-at nija-at assigned nija-at and unassigned nija-at Oct 29, 2021
@nija-at nija-at removed their assignment Nov 5, 2021
eladb
eladb previously approved these changes Nov 8, 2021
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify
Copy link
Contributor

mergify bot commented Nov 8, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot dismissed eladb’s stale review November 8, 2021 08:43

Pull request has been modified.

eladb
eladb previously approved these changes Nov 8, 2021
@mergify mergify bot dismissed eladb’s stale review November 8, 2021 08:54

Pull request has been modified.

@jogold jogold requested a review from eladb November 8, 2021 08:54
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: b0e07f5
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 90da730 into aws:master Nov 8, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 8, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@jogold jogold deleted the lambda-nodejs-custom-hash branch November 8, 2021 09:57
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
By default the asset hash for a `NodejsFunction` is based on the
bundling output, the result of the `esbuild` bundling.

Add a `assetHash` prop to customize the asset hash and allow users to
implement their own logic. This can be useful if users want to invalidate
the asset based on specific (input) file contents or logic.


Closes aws#16157


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-lambda-nodejs effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-lambda-nodejs): CUSTOM hash pass-through
4 participants