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
| Remote Link | New: This issue links to "PR 320 comments relevant to this ticket (Web Link)" [ 21951 ] |
| Assignee | Original: R. Tyler Croy [ rtyler ] |
| Labels | Original: evergreen newbie-friendly | New: evergreen newbie-friendly technical-debt |
| Labels | Original: evergreen newbie-friendly technical-debt | New: evergreen newbie-friendly |
| Link |
New:
This issue is blocked by |
| Description |
Original:
This is in reference to [this comment|[https://github.com/jenkins-infra/evergreen/pull/320#pullrequestreview-167387801].] 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. |
New:
This is in reference to [this comment|[https://github.com/jenkins-infra/evergreen/pull/320#pullrequestreview-167387801].] 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. h3. 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. h3. 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) |
Jen Lijó
made changes -
| Assignee | New: Jen Lijó [ jennyfive ] |
Jen Lijó
made changes -
| Comment |
[ [~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? ] |
| Labels | Original: evergreen newbie-friendly | New: evergreen |
| Resolution | New: Won't Do [ 10001 ] | |
| Status | Original: Open [ 1 ] | New: Closed [ 6 ] |
