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: add support for copying file permissions (options.copyPermissions) #119

Closed
wants to merge 3 commits into from

Conversation

deryni
Copy link

@deryni deryni commented Mar 14, 2017

Add support for copying permissions for copied files.

Use a copyPermissions: true key in a copy pattern to turn it on.

Should resolve #35.

@deryni
Copy link
Author

deryni commented Mar 14, 2017

I tried writing tests for this but ran into trouble getting the stats for the emitted files.

(I can't seem to find them post-webpack to get the stats from. Code here.)

@Antrikshy
Copy link

I'm setting up a new stack for an internal project at work that relies on this plugin. I'd love to see this pulled because we're currently relying on a slightly unsightly workaround for files that need to be copied with permissions.

@j0k3r
Copy link

j0k3r commented May 2, 2017

Definitely needs this in case you want to use executable file inside your lambda 👍

src/writeFile.js Outdated

perms |= stat.mode & fs.constants.S_IRWXU;
perms |= stat.mode & fs.constants.S_IRWXG;
perms |= stat.mode & fs.constants.S_IRWXO;
Copy link

Choose a reason for hiding this comment

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

These constants (S_IRWXU, S_IRWXG, S_IRWXO) doesn't exist in node < 6.3 😕

Copy link
Author

Choose a reason for hiding this comment

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

Do they exist in the constants module (i.e. require("constants") in the earlier versions?

Copy link

Choose a reason for hiding this comment

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

Yep they do exist in constants.default.S_IRWXU for example

Copy link

Choose a reason for hiding this comment

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

I've tested with:

// src/writeFile.js
perms |= stat.mode & (fs.constants.S_IRWXU || constants.default.S_IRWXU);

and

// src/index.js
let mask
if (fs.constants !== undefined) {
  mask = fs.constants.S_IRWXU | fs.constants.S_IRWXG | fs.constants.S_IRWXO;
} else {
  mask = constants.default.S_IRWXU | constants.default.S_IRWXG | constants.default.S_IRWXO;
}

And it works fine on node 6.3

@deryni deryni force-pushed the copy-file-permissions branch from 240e9c9 to b06b140 Compare June 26, 2017 16:29
deryni added 2 commits October 2, 2017 11:21
Add 'copyPermissions: true' to the entry for the files to enable.
@deryni deryni force-pushed the copy-file-permissions branch from b06b140 to 7e04f9c Compare October 2, 2017 15:22
@jsf-clabot
Copy link

jsf-clabot commented Oct 2, 2017

CLA assistant check
All committers have signed the CLA.

@michael-ciniawsky michael-ciniawsky changed the title Add support for copying file permissions on copied files. feat: add support for copying file permissions (options.copyPermissions) Oct 5, 2017
@michael-ciniawsky
Copy link
Member

@deryni Please rebase to latest master before we can triage this further :)

@michael-ciniawsky
Copy link
Member

Closing due to inactivity, feel free to reopen :)

@j0k3r
Copy link

j0k3r commented Nov 3, 2017

@michael-ciniawsky can't you retrieve its commits and do the rebase on your own?
We are using a fork with its modification and it works pretty good so far.

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Nov 3, 2017

@j0k3r Feel free to open a PR with these changes. This still needs review, e.g does it really need to be an option or could it be a default instead etc ? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] keeping original whole or partial stats
5 participants