This is in reference to this comment
The error handling for plugin and directory deletions should fail silently if file or directory not found. However, if we get any other error when attempting deletions, then the method should return an error.
Acceptance criteria:
- a test triggering showing that we now return an error if the file cannot be deleted and is not already absent
- (removing Write Permission on the file during the test seems like a good candidate?)
- In such case, the logging should be as clear as possible.
Open question
Should we have our own deleteFile variant that would try many steps to delete something before abandoning? (I am inclined to think so)
- is blocked by
-
JENKINS-54545 Promisify the plugin deletion
-
- Resolved
-
- links to
[JENKINS-54207] Update delete plugin flow to error out for anything other than file not found
Marking this as blocked by JENKINS-54545, even if not absolutely the case. But it would probably be cleaner and more readable to handle errors and avoid a bit of duplication after we've promisified the code.
tonho, sorry missed your comment. This issue is about Jenkins Evergreen, whose code is https://github.com/jenkins-infra/evergreen. The language is currently (Node) Javascript and Typescript.
For this very issue, this is going to be Typescript. But even if you do not know it well, and your goal is to learn, then very likely JENKINS-54545 first, then this one here are absolute great opportunities (the overall expected number of lines is going to be less than 50 modified lines for the former, and likely a dozen or so for the latter).
Please come over our chat on https://gitter.im/jenkins-infra/evergreen so that we can help you. Likely you want to look at https://github.com/jenkins-infra/evergreen/blob/master/HACKING.adoc
Thanks a bunch!
batmat asmith_cb, for the test, removing permissions on a file won't be enough to throw an error, as the `remove` method from `fs-extra` performs under the hood as `rm -rf` ( https://github.com/jprichardson/node-fs-extra/blob/master/docs/remove.md ).
On the other hand, seems like permissions to delete a file are dependant on permissions of its parent folder and not so much on the file itself. Changing the permission on the folder and then trying to delete the folder with a file inside might error with `Directory not empty`, but `fs.chmodSync(path, mode)` seems to not work with directories :/
EISDIR: illegal operation on a directory, open '/evergreen/data/jenkins/home/plugins/dir'
61 | fs.mkdirSync(`${pluginPath}/dir`);
62 | h.touchFile(`${pluginPath}/dir/somefile`);
> 63 | fs.chmodSync(`${pluginPath}/dir`, 444);
| ^ 64 |
expect(() =>
{
65 | Storage.removePlugins(['dir']);
66 | }
).toThrow(); |
Do you have any other possible scenario to throw an error on plugin deletion?
Jen Lijó
added a comment - - edited batmat asmith_cb , for the test, removing permissions on a file won't be enough to throw an error, as the `remove` method from `fs-extra` performs under the hood as `rm -rf` ( https://github.com/jprichardson/node-fs-extra/blob/master/docs/remove.md ).
On the other hand, seems like permissions to delete a file are dependant on permissions of its parent folder and not so much on the file itself. Changing the permission on the folder and then trying to delete the folder with a file inside might error with `Directory not empty`, but `fs.chmodSync(path, mode)` seems to not work with directories :/
EISDIR: illegal operation on a directory, open '/evergreen/data/jenkins/home/plugins/dir'
61 | fs.mkdirSync(`${pluginPath}/dir`);
62 | h.touchFile(`${pluginPath}/dir/somefile`);
> 63 | fs.chmodSync(`${pluginPath}/dir`, 444);
^
64
expect(() =>
{
65 | Storage.removePlugins(['dir']);
66 | }
).toThrow();
Do you have any other possible scenario to throw an error on plugin deletion? 
i would like to work on this issue, anyone can guide me ? im a little bit new with java, and i have only 1 PR on jenkins repo
thanks