Added support for changing uid/gid (fix #157) - #188
Conversation
| } | ||
|
|
||
| function getOwnerDiff(fsStat, vinylStat) { | ||
| if (/^win/.test(process.platform)) { |
There was a problem hiding this comment.
there should be no reason to do this. In updateMetadata, we exit early on windows (it doesn't have process.getuid())
|
@stephanvl very nice work. I just had some suggestions and the appveyor CI is still failing. Let me know once you've had a chance to clean it up. |
|
@phated I removed the latter 2 commits which added the windows specific code, added a I also updated the |
| }; | ||
| var expected = { | ||
| uid: 1001, | ||
| gid: undefined, |
There was a problem hiding this comment.
fs.fchown throws if uid or gid is undefined. Do we need to handle this by returning undefined if we don't have a complete diff?
|
@stephanvl this looks near perfect. Just 1 thing that I noticed while poking around in the node repl. Commented inline. |
|
@phated Good catch, I will change it to return undefined when either uid or gid is undefined as there is no way of knowing what it should be at that point. Then there is only the question what we should do about the fact that chown only works when the user is root (fail silently, throw an error or log a message and continue)? |
|
@stephanvl we already handle that with our |
|
@phated but you must be root for chown to work, being the owner of a file is not enough. |
|
Those errors should be bubbled up, I believe. |
|
graceful-fs gracefully ignores EPERM errors, which changing the owner as a non root user is https://github.com/isaacs/node-graceful-fs/blob/master/polyfills.js#L245-L249 |
|
👍 I'm fine with that then |
|
@stephanvl amazing work! Thanks a ton |
2 participants
This PR adds support for changing the uid/gid of a file/directory.
Changing the uid/gid of a file is only possible as the root user and will fail silently if this run as a non root user.
What do you think we should do in this case?