-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(lambda-python): bundle asset files correctly #18335
Conversation
cc @njlynch |
Never mind, I figured out how to add those tests and added them. Also added assertions for checking if hidden files get copied over (they do). |
fea3485
to
5af61ac
Compare
5af61ac
to
86c394c
Compare
86c394c
to
1ecaee6
Compare
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a reproducible example for what's still breaking for people? I worry we're throwing darts at the map (or flags on the CLI) and hoping it will fix it without truly understanding the root cause.
@@ -87,7 +87,7 @@ export class Bundling implements CdkBundlingOptions { | |||
if (packaging.dependenciesFile) { | |||
bundlingCommands.push(`python -m pip install -r ${DependenciesFile.PIP} -t ${options.outputDir}`); | |||
} | |||
bundlingCommands.push(`cp -R ${options.inputDir}/ ${options.outputDir}`); | |||
bundlingCommands.push(`cp -a ${options.inputDir}/* ${options.outputDir}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Including the *
still drops hidden files. This needs to be removed.
bundlingCommands.push(`cp -a ${options.inputDir}/* ${options.outputDir}`); | |
bundlingCommands.push(`cp -a ${options.inputDir}/ ${options.outputDir}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Urm, I added a validation for it within the test, which passes.
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). |
That's what I added assertions to validate. I'm confident those would fail for what's currently on the release versions because it would include an extra layer in between. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
Asset files are incorrectly being bundled under the `asset-input` directory instead of the root of the bundle. To also copy over hidden files (aws#18306 (comment)), I switched from using `-R` to `-a` based on what I found on [SO](https://stackoverflow.com/a/13020113) and the [man page](https://linux.die.net/man/1/cp). (`-a` is equivalent to `-dR`.) Fixes aws#18301 and @chrispykim's comment: aws#18082 (comment). ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Asset files are incorrectly being bundled under the
asset-input
directory instead of the root of the bundle.To also copy over hidden files (#18306 (comment)), I switched from using
-R
to-a
based on what I found on SO and the man page. (-a
is equivalent to-dR
.)Fixes #18301 and @chrispykim's comment: #18082 (comment).
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license