Skip to content

Add support for pre_uninstall scripts #2311

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 5 commits into from
Sep 18, 2023

Conversation

MatteoPologruto
Copy link
Contributor

@MatteoPologruto MatteoPologruto commented Sep 11, 2023

Please check if the PR fulfills these requirements

See how to contribute

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • configuration.schema.json updated if new parameters are added.

What kind of change does this PR introduce?

Code enhancement

What is the new behavior?

Support for pre_uninstall scripts has been added. The default behaviour of the CLI can be altered using the flags --run-pre-uninstall and --skip-pre-uninstall.

Does this PR introduce a breaking change, and is titled accordingly?

@MatteoPologruto MatteoPologruto added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Sep 11, 2023
@MatteoPologruto MatteoPologruto self-assigned this Sep 11, 2023
@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Patch coverage: 74.28% and project coverage change: +0.11% 🎉

Comparison is base (55fc5aa) 63.03% compared to head (cc4fc16) 63.14%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2311      +/-   ##
==========================================
+ Coverage   63.03%   63.14%   +0.11%     
==========================================
  Files         200      200              
  Lines       19206    19260      +54     
==========================================
+ Hits        12106    12162      +56     
+ Misses       6053     6052       -1     
+ Partials     1047     1046       -1     
Flag Coverage Δ
unit 63.14% <74.28%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
commands/core/uninstall.go 50.00% <50.00%> (ø)
internal/cli/arguments/pre_post_script.go 53.48% <60.00%> (ø)
arduino/cores/packagemanager/install_uninstall.go 58.00% <60.97%> (+2.59%) ⬆️
commands/core/install.go 71.42% <100.00%> (ø)
commands/core/upgrade.go 80.64% <100.00%> (ø)
internal/cli/core/install.go 93.47% <100.00%> (+0.14%) ⬆️
internal/cli/core/uninstall.go 76.31% <100.00%> (+3.58%) ⬆️
internal/cli/core/upgrade.go 81.81% <100.00%> (+0.23%) ⬆️
internal/cli/upgrade/upgrade.go 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MatteoPologruto MatteoPologruto marked this pull request as ready for review September 11, 2023 15:36
@MatteoPologruto MatteoPologruto linked an issue Sep 11, 2023 that may be closed by this pull request
3 tasks
Copy link
Contributor

@umbynos umbynos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I would only add a test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see some code duplication here, this could be an excellent use of subtests

@MatteoPologruto MatteoPologruto merged commit 29c70df into arduino:master Sep 18, 2023
@MatteoPologruto MatteoPologruto deleted the pre-uninstall-script branch September 18, 2023 13:09
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please support running a pre_uninstall script
2 participants