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

[EPM] Update UI to handle package versions and updates #64689

Merged
merged 10 commits into from
Apr 29, 2020

Conversation

neptunian
Copy link
Contributor

@neptunian neptunian commented Apr 28, 2020

#64255

  • Does not check for whether the user has auto updates turned on or make any mention of it because auto updating functionality doesn't exist yet and unlikely will for Alpha
  • Update the Settings tab for package detail page to display version information and update option
  • When the user clicks on a package from the list page they will go to the version they have installed
  • Fixes a bug where delete promises of saved objects assets were not being returned and awaited and resolving after some assets were installed so that the next time a delete or reinstall occurred, they did not exist

Testing instructions:

This scenario where user's have out of date packages in the UI would happen if the user had auto updates turned off which will be the case for alpha but the plan is to make auto updates default.

    • Go to the package list page and click on the "installed integrations" tab.
    • Click on the installed Nginx package and notice you are redirected to the version installed (0.0.1) and not the latest (0.0.2)
    • Try to update the package

User has the most up to date version of package installed
Screen Shot 2020-04-28 at 2 19 09 PM

User has out of date version installed
Screen Shot 2020-04-28 at 2 22 01 PM

User clicks the "update" button. Uninstall button is hidden so they don't try to uninstall during an update
Screen Shot 2020-04-28 at 2 22 42 PM

A successful update. they are redirected to the new package version page (change route) and there is a success toast
Screen Shot 2020-04-28 at 2 23 54 PM

An unsuccessful update attempt
Screen Shot 2020-04-28 at 2 42 41 PM

Edgecases where the user navigates to a version that is not the one they have installed (perhaps from a bookmark or history as we always link to the version they have installed or the most recent if not installed).

User goes to package version that is newer than the one they have installed. I am not too concerned about this view because they would have had to guess this URL as they always get sent to the version they have installed if they have one installed and update from that version
Navigate to http://localhost:5601/ssg/app/ingestManager#/epm/detail/nginx-0.0.2 with nginx-0.0.1 installed
Screen Shot 2020-04-28 at 2 49 30 PM

Navigate to http://localhost:5601/ssg/app/ingestManager#/epm/detail/nginx-0.0.1 with nginx-0.0.2 installed
User directly navigates to the url of a package that is older than the version they have installed. There are no options for installing or removing the package because its not the version they have installed.
Screen Shot 2020-04-28 at 2 56 25 PM

For the edgecases, Henry and I discussed possibility of changing the package detail route from /detail/nginx-0.0.1/ to /detail/nginx to simplify things, but this would be an issue for when we support installing multiple versions at once

@ruflin should we be blocking installations or updates to a version that is not the latest version?

@neptunian neptunian added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:EPM Fleet team's Elastic Package Manager (aka Integrations) project v7.8.0 Team:Fleet Team label for Observability Data Collection Fleet team labels Apr 28, 2020
@neptunian neptunian requested a review from a team April 28, 2020 19:02
@neptunian neptunian self-assigned this Apr 28, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@neptunian neptunian changed the title [EPM] Update UI to handle package versions [EPM] Update UI to handle package versions and updates Apr 28, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@skh
Copy link
Contributor

skh commented Apr 29, 2020

Can you add instructions how to test all the cases?

@ruflin
Copy link
Member

ruflin commented Apr 29, 2020

@neptunian What happens if a user will try to upgrade from the API? What are the API calls he has to make? Could you share an example?

@neptunian
Copy link
Contributor Author

neptunian commented Apr 29, 2020

@ruflin It's the same as installing:
http://localhost:5603/ssg/api/ingest_manager/epm/packages/nginx-0.0.1

Those changes were made as part of a different PR where the update and install became idempotent.

I was going to add the API restriction where they cannot update to a lower version as part of another PR.

@neptunian
Copy link
Contributor Author

@skh if you follow the user stories i have above each image, that was intended to be the instructions for testing. what did you have in mind?

@skh
Copy link
Contributor

skh commented Apr 29, 2020

@neptunian Basically, how do I install an outdated package in the first place? Through the API?

@neptunian
Copy link
Contributor Author

@skh good point. I updated the description with instructions

@neptunian
Copy link
Contributor Author

neptunian commented Apr 29, 2020

@skh after i took screenshots the registry appears to have been updated with nginx version 0.0.2 being added and 1.2.0 removed so wherever you see 1.2.0 use or think of 0.0.2

@ruflin
Copy link
Member

ruflin commented Apr 29, 2020

@neptunian We need to rethink the API here. Because someone using the API should not have to figure out first to which version he will upgrade or otherwise have an API endpoint that gives him this information. But let not block yourself on this and move forward with what you have.

@neptunian
Copy link
Contributor Author

neptunian commented Apr 29, 2020

@ruflin we need to change the routes to support this but we could do something like:
POST http://localhost:5603/ssg/api/ingest_manager/epm/packages/nginx or some specific update flag.
I am not sure whether to always take into mind the idea of having multiple packages installed at once because that impacts design a lot.

Copy link
Contributor

@skh skh left a comment

Choose a reason for hiding this comment

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

@neptunian Thank you for the updated description!

Works as described, nice job. 👍

Copy link
Contributor

@jfsiii jfsiii left a comment

Choose a reason for hiding this comment

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

Fantastic description with testing instructions and screenshots of user states / flows ✨

I left some comments but nothing too big.

I would like to avoid the new items in PackageAdditions, but we can come back to that later if at all.

Nice work

version,
panel: 'settings',
});
window.location.href = settingsUrl;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use useHistory or something besides window here, but this isn't the only place we do this so not a blocker.

@@ -43,6 +43,7 @@ export function createInstallableFrom<T>(
? {
...from,
status: InstallationStatus.installed,
installedVersion: savedObject.attributes.version,
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this available already using status and version independently?

Copy link
Contributor Author

@neptunian neptunian Apr 29, 2020

Choose a reason for hiding this comment

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

Here is an example of a response when you have 0.0.1 installed but GET api/ingest_manager/epm/packages/nginx-0.0.2. The version returned is the version you requested. The installedVersion is the saved object one and the latestVersion is the latest available. They can all be different. I put them in this endpoint because I didn't want to have to make different requests to get the latest or installed version.

Screen Shot 2020-04-29 at 1 34 54 PM

@@ -106,7 +106,7 @@ export async function installPackage(options: {
try {
await deleteKibanaSavedObjectsAssets(savedObjectsClient, installedPkg.attributes.installed);
} catch (err) {
// some assets may not exist if deleting during a failed update
// log these errors, some assets may not exist if deleted during a failed update
Copy link
Contributor

Choose a reason for hiding this comment

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

Add logging or a ticket for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what we should be logging. I've created another issue to track it #64814

@neptunian neptunian merged commit 53ff229 into elastic:master Apr 29, 2020
neptunian added a commit to neptunian/kibana that referenced this pull request Apr 29, 2020
* link to installed version of detail page

* add latestVersion property to EPM get package endpoint

* add updates available notices

* add update package button

* handle various states and send installedVersion from package endpoint

* fix type errors

* fix install error because not returning promises

* track version in state

* handle unsuccessful update attempt

* remove unused variable
neptunian added a commit that referenced this pull request Apr 30, 2020
* link to installed version of detail page

* add latestVersion property to EPM get package endpoint

* add updates available notices

* add update package button

* handle various states and send installedVersion from package endpoint

* fix type errors

* fix install error because not returning promises

* track version in state

* handle unsuccessful update attempt

* remove unused variable

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request May 4, 2020
…bana into pipeline-editor-part-mvp-2

* 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (90 commits)
  remove unused import
  address review feedback
  [Ingest pipelines] Cleanup (elastic#64794)
  [Ingest] Edit datasource UI (elastic#64727)
  [Lens] Bind all time fields to the time picker (elastic#63874)
  [Lens] Use suggestion system in chart switcher for subtypes (elastic#64613)
  Improve alpha messaging (elastic#64692)
  [Ingest] Allow to enable monitoring of elastic agent (elastic#63598)
  [Metrics UI] Fix alerting when a filter query is present (elastic#64575)
  skip flaky suite (elastic#64812) (elastic#64723)
  [Maps] do not display EMS or kibana layer wizards when not configured (elastic#64554)
  [Reporting/Test] Convert functional test code to Typescript (elastic#64601)
  make inserting timestamp with navigate methods optional with default true (elastic#64655)
  [EPM] Update UI to handle package versions and updates (elastic#64689)
  Minimize dependencies required by our telemetry middleware (elastic#64665)
  [Telemetry] oss api tests (elastic#64602)
  [ML] Adding endpoint capability checks (elastic#64662)
  Update jest config for coverage (elastic#64648)
  [SIEM][NP] Fixes bug in ML signals promotion (elastic#64720)
  share single data plugin bundle (elastic#64549)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:EPM Fleet team's Elastic Package Manager (aka Integrations) project release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants