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

API design: Uninstall software #21548

Closed
wants to merge 13 commits into from
Closed

API design: Uninstall software #21548

wants to merge 13 commits into from

Conversation

marko-lisica
Copy link
Member

@marko-lisica marko-lisica commented Aug 26, 2024

UPDATE: I closed this PR and opened a new one here (w/ same changes) against the docs-v4.57.0 branch.

As of 4.57.0, each minor release has it's own reference docs branch as part of a new process to make sure that every change to how Fleet is used is reflected live on the website in reference documentation at release day: https://github.com/fleetdm/fleet/pull/22284/files#diff-d426c2ae6cac2a2baffd54adae00ad7bb936dbb17a873f93a327d5763f7fb574R141


API design for: #20320


#### Example

`POST /api/v1/fleet/hosts/123/software/uinstall/3435`
Copy link
Member

Choose a reason for hiding this comment

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

typo *uninstall

Copy link
Member

Choose a reason for hiding this comment

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

TODO: change this to
POST /api/v1/fleet/hosts/123/software/3435/uninstall

And breaking change (because this is experimental) for existing install as well
POST /api/v1/fleet/hosts/123/software/3435/install

| pre_install_query | string | form | Query that is pre-install condition. If the query doesn't return any result, Fleet won't proceed to install. |
| post_install_script | string | form | The contents of the script to run after install. If the specified script fails (exit code non-zero) software install will be marked as failed and rolled back. |
| self_service | boolean | form | Self-service software is optional and can be installed by the end user. |
| uninstall_script | string | form | Script that Fleet runs to uninstall software. If not specified Fleet runs [default uninstall script](https://github.com/fleetdm/fleet/tree/f71a1f183cc6736205510580c8366153ea083a8d/pkg/file/scripts) for each package type. |

Copy link
Member

Choose a reason for hiding this comment

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

Should we add uninstall_script to the example below?

#### Example

`POST /api/v1/fleet/software/package`

##### Default response

`Status: 202`

Copy link
Member

Choose a reason for hiding this comment

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

Should we add uninstall_script_output to the default response below?

Copy link
Member

@RachelElysia RachelElysia left a comment

Choose a reason for hiding this comment

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

We should update API-for-contributors.md in several spots as well:

  • POST /api/v1/fleet/spec/teams
    1. software.packages description
    2. Example request body
  • POST /api/v1/fleet/software/batch
    3. software.packages description

@@ -9172,7 +9249,32 @@ _Available in Fleet Premium._

Install software (package or App Store app) on a macOS, iOS, iPadOS, Windows, or Linux (Ubuntu) host. Software title must have a `software_package` or `app_store_app` added to be installed.

`POST /api/v1/fleet/hosts/:id/software/install/:software_title_id`
`POST /api/v1/fleet/hosts/:id/software/:software_title_id/install`
Copy link
Member

Choose a reason for hiding this comment

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

Proposed breaking change

Copy link
Contributor

Choose a reason for hiding this comment

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

Interpreting "proposed" as we want to do it, making the change now. If proposed means there is still conversation around it let us know please.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@jacobshandling Yes, we are breaking this.

@rachaelshaw will do a final pass on API docs

docs/REST API/rest-api.md Outdated Show resolved Hide resolved
Co-authored-by: Rachael Shaw <r@rachael.wtf>
docs/REST API/rest-api.md Outdated Show resolved Hide resolved
Co-authored-by: Rachael Shaw <r@rachael.wtf>
Co-authored-by: jacobshandling <61553566+jacobshandling@users.noreply.github.com>
"failed_install": 0,
"pending_uninstall": 2,
"failed_uninstall": 1
}
"installed": 3,
Copy link
Member

Choose a reason for hiding this comment

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

TODO @RachelElysia: commit to remove old status (just applied Jacob's suggested changes that added correct statuses but didn't remove old status)

"host_display_name": "Marko’s MacBook Pro",
"software_title": "Adobe Acrobat.app",
"script_execution_id": "ecf22dba-07dc-40a9-b122-5480e948b756",
"status": "failed"
Copy link
Contributor

Choose a reason for hiding this comment

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

@getvictor @noahtalerman this "failed" value throws a bit of a curveball – having the potential values here match those from software titles / below would both make intuitive sense and help simplify/unify the frontend code:
export const SOFTWARE_INSTALL_STATUSES = [ "installed", "uninstalled", "pending_install", "failed_install", "pending_uninstall", "failed_uninstall", ] as const;

Does that make sense from your points of view?

Copy link
Member

Choose a reason for hiding this comment

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

Well, uninstalled is not a software install status. After software is uninstalled, the status goes to null, which means available for install.

Copy link
Contributor

@jacobshandling jacobshandling Sep 10, 2024

Choose a reason for hiding this comment

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

So I'll pull that out of this array. Do you think changing failed to failed_uninstall will work here?

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK with failed_uninstall. But what do you want as the activity status when uninstall succeeded?

Copy link
Contributor

Choose a reason for hiding this comment

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

uninstalled?

Copy link
Contributor

Choose a reason for hiding this comment

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

which is what it currently is:
Screenshot 2024-09-10 at 11 59 06 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

The array above, by the way, is being used by the frontend in a number of ways, including generating types for activities, not just to define the API response types

Copy link
Member

Choose a reason for hiding this comment

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

Hey @jacobshandling and @getvictor heads up that I think we went w/ one failed to be consistent w/ configuration profile status.

Failed install and failed removal for profiles are both failed:
Screenshot 2024-09-11 at 10 00 22 AM

Not sure if that still makes sense here but just wanted to call it out.

Copy link
Contributor

Choose a reason for hiding this comment

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

@noahtalerman thanks for the added context. I'll look into making it work as specified with "failed" today

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, should work with "failed" here cc @getvictor, no need to change it. Thanks all.

@@ -9183,7 +9290,7 @@ Install software (package or App Store app) on a macOS, iOS, iPadOS, Windows, or

#### Example

`POST /api/v1/fleet/hosts/123/software/install/3435`
`POST /api/v1/fleet/hosts/123/software/3435/uninstall`
Copy link
Contributor

Choose a reason for hiding this comment

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

BC

@@ -9193,7 +9300,7 @@ Install software (package or App Store app) on a macOS, iOS, iPadOS, Windows, or

_Available in Fleet Premium._

`GET /api/v1/fleet/software/install/results/:install_uuid`
`GET /api/v1/fleet/software/install/:install_uuid/results`
Copy link
Contributor

Choose a reason for hiding this comment

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

BC

@noahtalerman
Copy link
Member

Hey @marko-lisica heads up, I closed this PR and opened a new one here (w/ same changes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants