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

fix(toolkit): preserve asset's files mode #3413

Closed
wants to merge 3 commits into from

Conversation

aereal
Copy link

@aereal aereal commented Jul 24, 2019

The original file/directory permission (a.k.a. mode) of asset's files are lost since #2931 merged.

This problem breaks deployment of Lambda using Code.asset because compiled binary in the asset lacks executable bit.

After this P-R, zipDirectory pass mode argument that obtained from fs.stat.


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
    • Design: For significant features, design document added to design folder
  • Title and Description
    • Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

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

@aereal aereal requested review from RomainMuller and a team as code owners July 24, 2019 10:15
@jogold
Copy link
Contributor

jogold commented Jul 24, 2019

@aereal can you check if the solution proposed here #3145 (comment) preserves file mode (using archive.file)?

@aereal
Copy link
Author

aereal commented Jul 24, 2019

@jogold Sorry I missed that issue, I'll check that.

@jogold
Copy link
Contributor

jogold commented Jul 24, 2019

@jogold Sorry I missed that issue, I'll check that.

The issue is not about file mode but we are trying there to change the implementation of zipDirectory, would be great if we could satisfy all constraints at once.

@@ -4,6 +4,18 @@ import fs = require('fs-extra');
import glob = require('glob');
import path = require('path');

const appendFiles = async (directory: string, files: string[], archive: archiver.Archiver): Promise<void> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather declare this as async function appendFiles(directory: string, files: string[], archive: archiver.Archiver): Promise<void> { /*...*/ }

Copy link
Author

Choose a reason for hiding this comment

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

tslint tells me that async function violates non-arrow-function rule if function keyword used.

Even so should I do?

packages/aws-cdk/lib/archive.ts Outdated Show resolved Hide resolved
@aereal
Copy link
Author

aereal commented Jul 25, 2019

Fmm... I cannot reproduce failed test cases locally.

jogold added a commit to jogold/aws-cdk that referenced this pull request Jul 25, 2019
To preserve file order using `archiver` files must be appended serially either using stream or
buffer (appending by file path does not preserve order even when done serially).

Appending using buffer seems to be the only way to solve `EMFILE` errors.

Call `fs.stat` before appending to preserve mode.

Closes aws#3145, Closes aws#3344, Closes aws#3413
@eladb eladb closed this in #3428 Jul 25, 2019
eladb pushed a commit that referenced this pull request Jul 25, 2019
To preserve file order using `archiver` files must be appended serially either using stream or
buffer (appending by file path does not preserve order even when done serially).

Appending using buffer seems to be the only way to solve `EMFILE` errors.

Call `fs.stat` before appending to preserve mode.

Closes #3145, Closes #3344, Closes #3413
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