- Notifications
You must be signed in to change notification settings - Fork55
Add support for syncing file modes#19
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ea36792 tob72e29eCompare
SamKirkland left a comment
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.
Thanks for the PR. I've left some notes
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| // https://github.com/patrickjuchli/basic-ftp/issues/9 | ||
| letcommand="SITE CHMOD "+mode+" "+file.name | ||
| if(this.dryRun==false){ | ||
| awaitthis.client.ftp.request(command); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
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.
corrected
src/types.ts Outdated
| "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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should this be "new or updated files."?
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.
Mention this only works for unix based operating systems as well
timthelionFeb 13, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Are you sure stat doesn't work on windows? Nothing in the node docs about it not workinghttps://nodejs.org/api/fs.html#fspromisesstatpath-options and it's defined in libchttps://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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The client would load the permissions from the files. But the destination FTP server will have to be unix based in order for theCHMOD command to work.
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.
According to the docs I linked to earlier, it only works on FTP servers that supportCHMOD 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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."
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.
corrected
Uh oh!
There was an error while loading.Please reload this page.
timthelion commentedFeb 13, 2022
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/ | ||
| letstats=awaitstat(this.localPath+file.name); | ||
| letmode:string="0"+(stats.mode&parseInt('777',8)).toString(8); | ||
| // https://github.com/patrickjuchli/basic-ftp/issues/9 | ||
| letcommand="SITE CHMOD "+mode+" "+file.name | ||
| if(this.dryRun===false){ | ||
| awaitthis.client.ftp.request(command); | ||
| } | ||
| this.logger.verbose("Setting file mode with command "+command); |
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.
This method is correct, but not sure if this will work, because you are only calling thesyncMode 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 interfacesIFile andIFolder with a new attribute (i.e.
chmod) - When we are loading the local filesgetLocalFiles, you need to process the chmod attribute too.
- When the folders/filesare the same, you need to add the chmod comparison.
- Also maybe, I would add a new list inDiffResult with only
chmodchanges.
I guess this is a better approach for your problem, and this is according withftp-deploy project with the diff files comparison approach.
@SamKirkland thoughts?
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.
@JaviRpo That could be a fine idea. The correct name for such an attribute wouldn't bechmod butmode ;)chmod stands for "change mode".
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.
corrected. Looks like no need anymore
timthelion commentedJun 14, 2022
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
ihardz commentedOct 31, 2023
@SamKirkland , Could you please take a look at the PR now. Thanks |
See:SamKirkland/FTP-Deploy-Action#209