Add support for syncing file modes #19
Conversation
timthelion
force-pushed
the
file-modes
branch
3 times, most recently
from
ea36792
to
b72e29e
Compare
timthelion
changed the title
| // https://github.com/patrickjuchli/basic-ftp/issues/9 | ||
| let command = "SITE CHMOD " + mode + " " + file.name | ||
| if (this.dryRun == false) { | ||
| await this.client.ftp.request(command); |
There was a problem hiding this comment.
try/catch for servers that don't support this. I'm thinking we give a nice error message that explains the server doesn't support setting permissions - then rethrow so the action fails
There was a problem hiding this comment.
I'd have to learn how to do that on stack overflow, and I'd have no way to test it, because I don't have access to a a non-posix compiant FTP server. I'd like to preserve and display whatever error context there is, because an error here might not have anything to do with POSIX compliance, for example, it could be caused by a loss of connection. Is there a way to forward all the error context and add a message? Is there really a point to doing this? The option is already off by default.
src/types.ts
Outdated
| @@ -35,6 +35,12 @@ export interface IFtpDeployArguments { | |||
| */ | |||
| "dry-run"?: boolean; | |||
|
|
|||
| /** | |||
| * Tries to sync posix file modes to server. Only works for new and updated files. | |||
There was a problem hiding this comment.
Should this be "new or updated files."?
There was a problem hiding this comment.
Mention this only works for unix based operating systems as well
There was a problem hiding this comment.
Are you sure stat doesn't work on windows? Nothing in the node docs about it not working https://nodejs.org/api/fs.html#fspromisesstatpath-options and it's defined in libc https://codeforwin.org/2018/03/c-program-find-file-properties-using-stat-function.html so it should work on windows.
Edit: I did my development on debian 10 and will not test this on windows.
There was a problem hiding this comment.
The client would load the permissions from the files. But the destination FTP server will have to be unix based in order for the CHMOD command to work.
There was a problem hiding this comment.
According to the docs I linked to earlier, it only works on FTP servers that support CHMOD which is not the same as unix based FTP servers. There are both non unix based servers that support that command and unix based servers that don't support that command. That's why I write "tries to" rather than "sets" ;)
There was a problem hiding this comment.
In any case, in case you didn't notice, I did add the following on the line below: "Note: Not all FTP servers support settings POSIX file modes."
timthelion
force-pushed
the
file-modes
branch
from
b72e29e
to
2b07bcf
Compare
|
Thank you for the code review. I'm a total outside when it comes to nodejs so I've made all the changes except the |
src/syncProvider.ts
Outdated
| this.logger.verbose("Syncing posix mode for file " + file.name); | ||
| // https://www.martin-brennan.com/nodejs-file-permissions-fstat/ | ||
| let stats = await stat(this.localPath + file.name); | ||
| let mode: string = "0" + (stats.mode & parseInt('777', 8)).toString(8); | ||
| // https://github.com/patrickjuchli/basic-ftp/issues/9 | ||
| let command = "SITE CHMOD " + mode + " " + file.name | ||
| if (this.dryRun === false) { | ||
| await this.client.ftp.request(command); | ||
| } | ||
| this.logger.verbose("Setting file mode with command " + command); |
There was a problem hiding this comment.
This method is correct, but not sure if this will work, because you are only calling the syncMode function when you have differences in folder/files.
What if you move your change to another place? or change the perspective.
For example:
- I would improve the interfaces IFile and IFolder with a new attribute (i.e.
chmod) - When we are loading the local files getLocalFiles, you need to process the chmod attribute too.
- When the folders/files are the same, you need to add the chmod comparison.
- Also maybe, I would add a new list in DiffResult with only
chmodchanges.
I guess this is a better approach for your problem, and this is according with ftp-deploy project with the diff files comparison approach.
@SamKirkland thoughts?
There was a problem hiding this comment.
@JaviRpo That could be a fine idea. The correct name for such an attribute wouldn't be chmod but mode ;) chmod stands for "change mode".
|
Hi, In the mean time, you can use my fork ex: https://github.com/vegan-buddies/vegan-buddies/blob/20e437da95379930c7dd05cecd87f6fe1801fee0/.github/workflows/website.yml#L34 |
Fix: posix server support
|
@SamKirkland , Could you please take a look at the PR now. Thanks |
See: SamKirkland/FTP-Deploy-Action#209