Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): Add pre_uninstall and post_uninstall hooks #4957

Merged

Conversation

chawyehsu
Copy link
Member

@chawyehsu chawyehsu commented May 29, 2022

Description

Introducing pre_uninstall and post_uninstall hooks.

Motivation and Context

Closes #1868
Closes #4218

How Has This Been Tested?

hooktest1.json
{
    "version": "0.2.0",
    "description": "A description",
    "homepage": "https://scoop.sh",
    "license": "UNLICENSE-or-MIT",
    "url": "https://github.com/ScoopInstaller/Scoop/archive/refs/tags/v0.2.0.tar.gz",
    "hash": "2cb9c0db892d13fed5bc49507deadc9c98f1038a96edba2d71c33a91bdb984a7",
    "extract_dir": "Scoop-0.2.0",
    "pre_uninstall": "Write-Host 'hook pre_uninstall executed.'",
    "post_uninstall": "Write-Host 'hook post_uninstall executed.'"
}
.\bin\scoop.ps1 install .\hooktest1.json
.\bin\scoop.ps1 uninstall hooktest1
hooktest2.json
{
    "version": "0.2.0",
    "description": "A description",
    "homepage": "https://scoop.sh",
    "license": "UNLICENSE-or-MIT",
    "architecture": {
        "64bit": {
            "url": "https://github.com/ScoopInstaller/Scoop/archive/refs/tags/v0.2.0.tar.gz",
            "hash": "2cb9c0db892d13fed5bc49507deadc9c98f1038a96edba2d71c33a91bdb984a7",
            "extract_dir": "Scoop-0.2.0",
            "pre_uninstall": "Write-Host 'hook pre_uninstall executed.'",
            "post_uninstall": "Write-Host 'hook post_uninstall executed.'"
        }
    }
}
.\bin\scoop.ps1 install .\hooktest2.json
.\bin\scoop.ps1 uninstall hooktest2

Checklist:

  • I have read the Contributing Guide.
  • I have ensured that I am targeting the develop branch.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.
  • I have added an entry in the CHANGELOG.

Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
@chawyehsu chawyehsu marked this pull request as ready for review May 29, 2022 03:02
@rashil2000
Copy link
Member

What's the advantage of this over uninstaller.script?

@niheaven
Copy link
Member

post_uninstall is executed after all things done and may be needed, but does pre_uninstall do the same thing as uninstall.script?

@rashil2000
Copy link
Member

Also, the function signature and body for pre_install, post_install, pre_uninstall and post_uninstall is exactly the same. Why not just consolidate it into a single function and pass the scripts to that function as parameter?

@chawyehsu
Copy link
Member Author

chawyehsu commented May 29, 2022

Basically they have all the same functionality as the pre/post_install paired with installer.script. From my experience, installer.script can be used in most cases where pre_install is used, but users may use pre_install for reasons.

Therefore to me they make sense semantically.

Why not just consolidate it into a single function and pass the scripts to that function as parameter?

It's ok to do such refactoring.

@rasa
Copy link
Member

rasa commented May 29, 2022

Just an FYI, 34 of this bucket's ~300 manifiests use pre_uninstall.

Signed-off-by: Chawye Hsu <chawyehsu@hotmail.com>
@niheaven niheaven merged commit dd0f514 into ScoopInstaller:develop Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants