-
-
Notifications
You must be signed in to change notification settings - Fork 0
Fix dry_run bypass in install/update/uninstall and correct README #26
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
base: main
Are you sure you want to change the base?
Changes from all commits
67821c0
a676cc0
7756837
7abe208
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2126,8 +2126,6 @@ def install( | |
| ACTIVE_EXEC_LOG_PREFIX.reset(exec_log_prefix_token) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Dry-run checks are executed after handler calls, so install/update/uninstall still run provider code during dry runs. Prompt for AI agents |
||
|
|
||
| if self.dry_run: | ||
| # return fake ShallowBinary if we're just doing a dry run | ||
| # no point trying to get real abspath or version if nothing was actually installed | ||
| return ShallowBinary.model_construct( | ||
| name=bin_name, | ||
| description=bin_name, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -373,6 +373,14 @@ def default_install_handler( | |
| ansible_playbook = self.INSTALLER_BINARY(no_cache=no_cache).loaded_abspath | ||
| assert ansible_playbook | ||
|
|
||
| if self.dry_run: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The new dry-run check still runs after Prompt for AI agents |
||
| logger.info( | ||
| "DRY RUN (%s): ansible-playbook install %s", | ||
| self.__class__.__name__, | ||
| " ".join(install_args), | ||
| ) | ||
| return f"DRY RUN: would install {install_args} via ansible" | ||
|
|
||
| module_extra_kwargs = self.get_ansible_module_extra_kwargs() | ||
|
|
||
| return ansible_package_install( | ||
|
|
@@ -401,6 +409,14 @@ def default_update_handler( | |
| ansible_playbook = self.INSTALLER_BINARY(no_cache=no_cache).loaded_abspath | ||
| assert ansible_playbook | ||
|
|
||
| if self.dry_run: | ||
| logger.info( | ||
| "DRY RUN (%s): ansible-playbook update %s", | ||
| self.__class__.__name__, | ||
| " ".join(install_args), | ||
| ) | ||
| return f"DRY RUN: would update {install_args} via ansible" | ||
|
|
||
| module_extra_kwargs = self.get_ansible_module_extra_kwargs() | ||
| if module_extra_kwargs: | ||
| return ansible_package_install( | ||
|
|
@@ -439,6 +455,14 @@ def default_uninstall_handler( | |
| ansible_playbook = self.INSTALLER_BINARY(no_cache=no_cache).loaded_abspath | ||
| assert ansible_playbook | ||
|
|
||
| if self.dry_run: | ||
| logger.info( | ||
| "DRY RUN (%s): ansible-playbook uninstall %s", | ||
| self.__class__.__name__, | ||
| " ".join(install_args), | ||
| ) | ||
| return True | ||
|
|
||
| module_extra_kwargs = self.get_ansible_module_extra_kwargs() | ||
| if module_extra_kwargs: | ||
| ansible_package_install( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Moving dry_run check after handler call in
uninstall()causes real file deletions during dry_runThe base class
BinProvider.uninstall()previously short-circuited before calling the handler whenself.dry_runis true (abxpkg/binprovider.py:2380, old code). This PR moved the check to after the handler runs at line 2410. Whileself.exec()has its own dry_run guard (line 1699), multiple providers' uninstall handlers perform direct file operations that bypassexec()and will now execute even during dry_run:abxpkg/binprovider_goget.py:255):Path(abspath).unlink()deletes the binaryabxpkg/binprovider_docker.py:280-281): deletes wrapper script and metadata JSONabxpkg/binprovider_puppeteer.py:702,707): deletes symlink andshutil.rmtree()on browser dirabxpkg/binprovider_playwright.py:726,736): deletes symlink andshutil.rmtree()on browser dirsabxpkg/binprovider_chromewebstore.py:264-269): deletes cache, CRX, and unpacked extensionNone of these uninstall handlers have their own
dry_runchecks, unlike the Ansible/Pyinfra handlers that this PR explicitly patched. Aprovider.uninstall(bin_name, dry_run=True)call will now irreversibly delete files.(Refers to lines 2410-2411)
Prompt for agents
Was this helpful? React with 👍 or 👎 to provide feedback.