Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-54207

Update delete plugin flow to error out for anything other than file not found

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      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)

        Attachments

          Issue Links

            Activity

            Hide
            tonho Elton Alves added a comment -

            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

            Show
            tonho Elton Alves added a comment - 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
            Hide
            batmat Baptiste Mathus added a comment -

            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.

            Show
            batmat Baptiste Mathus added a comment - 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.
            Hide
            batmat Baptiste Mathus added a comment -

            Elton Alves, 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!

            Show
            batmat Baptiste Mathus added a comment - Elton Alves , 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!
            Hide
            jennyfive Jen Lijó added a comment - - edited

            Baptiste Mathus Mandie Smith, 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?

            Show
            jennyfive Jen Lijó added a comment - - edited Baptiste Mathus Mandie Smith , 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?
            Hide
            jennyfive Jen Lijó added a comment -

            PS: sorry for the horrible formatting..!! Can't manage to fix it :s

            Show
            jennyfive Jen Lijó added a comment - PS: sorry for the horrible formatting..!! Can't manage to fix it :s

              People

              • Assignee:
                jennyfive Jen Lijó
                Reporter:
                asmith_cb Mandie Smith
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated: