Uninstall packages #21892
Uninstall packages #21892
Conversation
…are modal (#21828) ## Addresses #21564 – see issue for task list     - [x] Manual QA for all new/changed functionality --------- Co-authored-by: Jacob Shandling <jacob@fleetdm.com>
…stalling" states (#21858) ## #21565   - [x] Manual QA for all new/changed functionality --------- Co-authored-by: Jacob Shandling <jacob@fleetdm.com>
## Follow-up to #21564     - [x] Manual QA for all new/changed functionality --------- Co-authored-by: Jacob Shandling <jacob@fleetdm.com>
# Conflicts: # server/datastore/mysql/schema.sql
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21892 +/- ##
==========================================
+ Coverage 64.96% 65.33% +0.37%
==========================================
Files 1492 1493 +1
Lines 116151 116321 +170
Branches 3502 3540 +38
==========================================
+ Hits 75454 75997 +543
+ Misses 33692 33238 -454
- Partials 7005 7086 +81
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Just so we don't forget, we still need the cron job to backfill uninstall scripts. Guessing that requires introspecting the entire installer, though maybe some shortcuts are available for some installer types? I'm going to need to do a subset of this to determine "real" installer extension because we don't store that and I need it for Edit to validate that the installer type isn't changing as a result of the user uploading. Looks like I could just pull the first 64 bytes though and that would be enough information to determine the installer type. |
|
Actually, potentially never mind on the optimizations...PE files look to have an offset value at 62 bytes, and then we have to look at what's at that offset. So I guess we pull everything. Thoughts on adding an extension column in the DB as part of this work, and populating that as you're doing the uninstaller backfill? Grabbing every installer feels like a thing we want to do once... |
|
Also seems like the uninstall script preprocess function would be easier to test/reuse if it specified a tighter interface and/or didn't change the script in-place? I'm probably overlooking why the way it is right now is better though. |
| @@ -89,6 +89,8 @@ type SoftwareInstaller struct { | |||
| InstallScript string `json:"install_script" db:"install_script"` | |||
| // InstallScriptContentID is the ID of the install script content. | |||
| InstallScriptContentID uint `json:"-" db:"install_script_content_id"` | |||
| // UninstallScriptContentID is the ID of the uninstall script content. | |||
| UninstallScriptContentID uint `json:"-" db:"uninstall_script_content_id"` | |||
There was a problem hiding this comment.
I think we also need UninstallScript here? We need uninstall script visible/editable in the places where we need e.g. InstallScript visible/editable.
There was a problem hiding this comment.
...yeah, 99.99% sure we do. Going to see if I can comit this to my branch so you can cherry-pick upstream if I did the work correctly.
I created a subtask for this work. It can be done as a separate PR. #21903 |
I don't understand what we're validating here. We have the installer filename saved in |
The PACKAGE_ID may not be correct. For example, a PKG installer may have 2 package IDs, but install only 1. So the user needs to be able to edit the actual package IDs that will be uninstalled. |
Also hydrate uninstall script content ID when pulling installers when uninstall script contents aren't requested
# Conflicts: # server/datastore/mysql/schema.sql # server/fleet/software_installer.go
getvictor
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
getvictor
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
getvictor
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
getvictor
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
| return fleet.NewInvalidArgumentError("software_title_id", `No uninstall script exists for the provided "software_title_id".`). | ||
| WithStatus(http.StatusNotFound) | ||
| } | ||
| return err |
There was a problem hiding this comment.
Ah I meant using ctxerr on line 707, but this is just a nit.
pkg/file/scripts/install_exe.ps1
Outdated
| # Start process and track exit code | ||
| $process = Start-Process @processOptions | ||
| $exitCode = $process.ExitCode | ||
|
|
||
| # Start process and track exit code | ||
| $process = Start-Process @processOptions | ||
| $exitCode = $process.ExitCode |
There was a problem hiding this comment.
Maybe copy/paste or merge issue. Will fix.
| # Get volume and location of package | ||
| volume=$(pkgutil --pkg-info "$pkg_id" | grep -i "volume" | awk '{for (i=2; i<NF; i++) printf $i " "; print $NF}') | ||
| location=$(pkgutil --pkg-info "$pkg_id" | grep -i "location" | awk '{for (i=2; i<NF; i++) printf $i " "; print $NF}') | ||
| # Check if this package id corresponds to a valid/installed package |
There was a problem hiding this comment.
Should it echo some warning if volume or location are empty?
There was a problem hiding this comment.
There's an existing pkg/file/testdata/installers/ (empty with a .gitignore)
There was a problem hiding this comment.
Yes, it is used by another test, so I couldn't put my file there. I didn't dig into the other test.
| si.uninstall_script_content_id, | ||
| si.uploaded_at, | ||
| si.uninstall_script_content_id, |
| @@ -732,6 +800,12 @@ WHERE global_or_team_id = ? | |||
| } | |||
| installScriptID, _ := isRes.LastInsertId() | |||
|
|
|||
| uisRes, err := insertScriptContents(ctx, tx, installer.UninstallScript) | |||
| if err != nil { | |||
| return ctxerr.Wrapf(ctx, err, "inserting install script contents for software installer with name %q", installer.Filename) | |||
| ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { | ||
| _, err = q.ExecContext(ctx, ` | ||
| UPDATE host_software_installs SET uninstall = 1 WHERE host_id = ? AND software_installer_id = ?`, | ||
| hostPendingUninstall.ID, si.InstallerID) | ||
| require.NoError(t, err) | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
Isn't this done in ds.InsertSoftwareUninstallRequest above?
| ADD COLUMN uninstall_script_exit_code INT DEFAULT NULL, | ||
| ADD COLUMN uninstall TINYINT UNSIGNED NOT NULL DEFAULT 0, | ||
| ADD COLUMN status ENUM('pending_install', 'failed_install', 'installed', 'pending_uninstall', 'failed_uninstall') | ||
| GENERATED ALWAYS AS ( |
There was a problem hiding this comment.
Thanks for moving this logic as a generated column!
getvictor
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
…activity feed, misc other items (#21933) ## #21566 - Host details updates for Uninstall packages details > software page. Full tasks outlined in the issue, [Figma here](https://www.figma.com/design/ToQaK2yUJwDyzagTdrbOfX/%2320320-Uninstall-packages?node-id=5364-13173&m=dev) **Updated install status tooltips:**  **Uninstall action:**  **Update install details:**  ## #21931 - updated specs for install/uninstall states ## #21568 - activity feed items for Uninstall    ## #21567 - Uninstall details modal  _remaining TODO_: - [x] manually QA 'failed' states - [x] determine where to source timestamp from for uninstall activities - [x] Manual QA for all new/changed functionality --------- Co-authored-by: Jacob Shandling <jacob@fleetdm.com> Co-authored-by: Victor Lyuboslavsky <victor@fleetdm.com>
jacobshandling
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
#20320
Demo video(s)
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)COLLATE utf8mb4_unicode_ci).