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
[Fleet] Update Package Policy UI to Support Upgrading Package Policies #107171
[Fleet] Update Package Policy UI to Support Upgrading Package Policies #107171
Conversation
c4c6e91
to
7a7072d
Compare
@jen-huang @nchaulet - I was working on adding the "previous configuration" flyout to this UI when I came across a question. When we have an input variable removed between versions (e.g. I removed "KP Test 4" in this latest version of the See screen recording for example: Maybe I'm misunderstanding, but do we need to display the union of both sets of variables between the previous and next version of the package? We have some general thinking around field level conflicts in the designs here, for context: Let me know if you have thoughts here. |
@kpollich 👍 on using the next package version as the source of truth for the policy builder variables. My guess here is if a package variable is deleted we should automatically remove the var for the user in the upgrade API I do not think there is a need to keep unused variables. |
</EuiFlyoutHeader> | ||
<FlyoutBody> | ||
<EuiCodeBlock isCopyable fontSize="m" whiteSpace="pre"> | ||
{JSON.stringify(currentPackagePolicy, null, 2)} |
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.
I am wondering here if it will make more sense for the user to show the compiled package policy as it's sent to the agent? we never show the package policy and I think we should probably hide that complexity to the user.
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.
Yeah definitely. Is there a utility for compiling it into that format?
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.
This is something that I think requires further discussion. The API that was set up in #103017 will return an error if a variable is removed between versions. Right now, this is our only notion of "conflict detection", as far as I can tell. I believe this error surfaces when we attempt to compile the package inputs for the next version here: kibana/x-pack/plugins/fleet/server/services/package_policy.ts Lines 658 to 697 in 58054c3
|
@elasticmachine merge upstream |
In my opinion here if a variable is removed, and no new variable are required we should probably not throw an error what do you think? As a user I want to have the smoothest experience possible and if I can avoid to manage conflict it's probably better no? If we go in this direction this should probably be documented for packages maintainer |
I spoke with @jen-huang about this a bit, and I think our approach will be to surface this a non-blocking error. e.g. just an "info" level callout that says something like.
cc @dborodyansky - thoughts on this case? Non-conflicting notices before a user upgrades. |
…ana into upgrade-package-policies/ui
@elasticmachine merge upstream |
...ic/applications/fleet/sections/agent_policy/create_package_policy_page/components/layout.tsx
Outdated
Show resolved
Hide resolved
...ic/applications/fleet/sections/agent_policy/create_package_policy_page/components/layout.tsx
Outdated
Show resolved
Hide resolved
...ins/fleet/public/applications/fleet/sections/agent_policy/edit_package_policy_page/index.tsx
Outdated
Show resolved
Hide resolved
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsAPI count
API count missing comments
History
To update your PR or re-run it, just comment with: cc @kpollich |
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.
Looks good to me, did some tests locally and everything worked 🚀
elastic#107171) * Remove description column, replace w/ tooltip * Add upgrade tooltip to version column in package policy table * Add update policy action * Add inline upgrade package policy button * Clean up types + add upgrade CTA's to integrations policy table * Fix i18n * Fix button widths * Upgrade package policy when saving integration w/ upgrade param * Update edit policy page description for upgrades * Support setting vars on new package version before saving to upgrade * Add flyout for JSON of previous policy version * Compile package policy before displaying in flyout * Support different success redirects following package policy upgrades * Fix i18n * Fix more type errors * Fix even more type errors 🙃 * Fix type errors * Don't throw errors for missing vars, include them in missingVars response object * Update tests for new missingVars field * Fix failing tests * Address PR feedback * Fix missing i18n value * Fix types Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
#107171) (#108204) * Remove description column, replace w/ tooltip * Add upgrade tooltip to version column in package policy table * Add update policy action * Add inline upgrade package policy button * Clean up types + add upgrade CTA's to integrations policy table * Fix i18n * Fix button widths * Upgrade package policy when saving integration w/ upgrade param * Update edit policy page description for upgrades * Support setting vars on new package version before saving to upgrade * Add flyout for JSON of previous policy version * Compile package policy before displaying in flyout * Support different success redirects following package policy upgrades * Fix i18n * Fix more type errors * Fix even more type errors 🙃 * Fix type errors * Don't throw errors for missing vars, include them in missingVars response object * Update tests for new missingVars field * Fix failing tests * Address PR feedback * Fix missing i18n value * Fix types Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kyle Pollich <kyle.pollich@elastic.co>
Summary
This PR encapsulates the "Upgrade package policy UI" milestone laid out in #106048. It adds support for upgrading package policies to the latest installed version of a given integration.
See Figma Designs
Screen Recording
Fleet > Agent policies > Agent policy details
integrations table to matchdesign
Upgrade policy
action menu item if the policy package version =/= the currently installed package versionIntegrations > Integration details > Policies
tableUpgrade policy
action menu item if the policy package version =/= the currently installed package versionUpgrade policy
, take them to the policy editorprevious configuration
should open a flyout with the JSON value of the existing package policy to enable users to copy over previous values (diff
field from API)Save integration
, save the adjusted policy with the new package version on the package metadataTesting Instructions
xpack.fleet.registryUrl: http://localhost:8080
in yourkibana.dev.yml
. Also ensure you have https://github.com/elastic/integrations cloned locally and https://github.com/elastic/elastic-package installed.apache
integration to your default agent policyapache
package in itsmanifest.yml
apache
package viaelastic-package build
apache
package version to your package registry'sbuild
directory, e.g.cp -R ../integrations/build/integrations/apache/<new_version> ./build/package-storage/packages/apache
apache
via theIntegrations -> Manage -> Apache -> Settings
screenIntegrations -> Manage -> Apache -> Policies
orFleet -> Agent Policies -> Default Policy -> Integrations
screensapache
package policy, or selectUpgrade Policy
from the actions menu in the table