-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Snapshot Restore] Fix initial policy form state #83928
[Snapshot Restore] Fix initial policy form state #83928
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
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.
Tested locally, code LGTM! Had one suggestion.
const { policy: policyToEdit } = policyData; | ||
|
||
// The policy response includes data not pertinent to the form | ||
// that we need to remove, e.g., lastSuccess, lastFailure, stats |
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.
👍 Thanks for this comment!
}) | ||
.expect(200); | ||
|
||
expect(body).to.eql({ |
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.
For the "create" and "update" tests, would it be worth fetching the policy and verifying that it's been created/updated? Otherwise we know that our API has responded as if everything's great, but we haven't verified that integration with ES behaved as expected.
I also don't see a "delete" test, should we add one?
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.
For the "create" and "update" tests, would it be worth fetching the policy and verifying that it's been created/updated? Otherwise we know that our API has responded as if everything's great, but we haven't verified that integration with ES behaved as expected.
Good point! Added this check for the tests.
I also don't see a "delete" test, should we add one?
I'm going to hold off on this for now, as it feels outside the scope of the initial PR. There are some tests already that run using mocks.
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
* master: (40 commits) fix: 🐛 don't add separator befor group on no main items (elastic#83166) [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323) [APM] Add APM agent config options (elastic#84678) Fixed a11y issue on rollup jobs table selection (elastic#84567) [Discover] Refactor getContextUrl to separate file (elastic#84503) [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654) [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749) [Lens] Migrate legacy es client and remove total hits as int (elastic#84340) Improve logging pipeline in @kbn/legacy-logging (elastic#84629) Catch @hapi/podium errors (elastic#84575) [Discover] Unskip date histogram test (elastic#84727) Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791) [Enterprise Search] Fix schema errors button (elastic#84842) [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589) [Maps] Always initialize routes on server-startup (elastic#84806) [Fleet] EPM support to handle uploaded file paths (elastic#84708) [Snapshot Restore] Fix initial policy form state (elastic#83928) Upgrade Node.js to version 14 (elastic#83425) [Security Solution] Keep Endpoint policies up to date with license changes (elastic#83992) [Security Solution][Exceptions] Implement exceptions for ML rules (elastic#84006) ...
* master: (236 commits) fix: 🐛 don't add separator befor group on no main items (elastic#83166) [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323) [APM] Add APM agent config options (elastic#84678) Fixed a11y issue on rollup jobs table selection (elastic#84567) [Discover] Refactor getContextUrl to separate file (elastic#84503) [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654) [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749) [Lens] Migrate legacy es client and remove total hits as int (elastic#84340) Improve logging pipeline in @kbn/legacy-logging (elastic#84629) Catch @hapi/podium errors (elastic#84575) [Discover] Unskip date histogram test (elastic#84727) Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791) [Enterprise Search] Fix schema errors button (elastic#84842) [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589) [Maps] Always initialize routes on server-startup (elastic#84806) [Fleet] EPM support to handle uploaded file paths (elastic#84708) [Snapshot Restore] Fix initial policy form state (elastic#83928) Upgrade Node.js to version 14 (elastic#83425) [Security Solution] Keep Endpoint policies up to date with license changes (elastic#83992) [Security Solution][Exceptions] Implement exceptions for ML rules (elastic#84006) ...
…overy-action-group * upstream/master: (48 commits) [Lens] accessibility screen reader issues (elastic#84395) [Logs UI] Fetch single log entries via a search strategy (elastic#81710) fix: 🐛 don't add separator befor group on no main items (elastic#83166) [Security Solution][Detections] Implements indicator match rule cypress test (elastic#84323) [APM] Add APM agent config options (elastic#84678) Fixed a11y issue on rollup jobs table selection (elastic#84567) [Discover] Refactor getContextUrl to separate file (elastic#84503) [Embeddable] Export CSV action for Lens embeddables in dashboard (elastic#83654) [TSVB] [Cleanup] Remove extra dateFormat props (elastic#84749) [Lens] Migrate legacy es client and remove total hits as int (elastic#84340) Improve logging pipeline in @kbn/legacy-logging (elastic#84629) Catch @hapi/podium errors (elastic#84575) [Discover] Unskip date histogram test (elastic#84727) Rename server.xsrf.whitelist to server.xsrf.allowlist (elastic#84791) [Enterprise Search] Fix schema errors button (elastic#84842) [APM] Removes react-sticky dependency in favor of using CSS (elastic#84589) [Maps] Always initialize routes on server-startup (elastic#84806) [Fleet] EPM support to handle uploaded file paths (elastic#84708) [Snapshot Restore] Fix initial policy form state (elastic#83928) Upgrade Node.js to version 14 (elastic#83425) ...
Fixes #83862
The UI fetches the selected policy to populate the fields in the edit policy wizard. The response includes data unnecessary to the form (stats, last success/last failure information). This was all getting sent back to the server on save, which can cause an error if the payload content is too large.
Release note
This fixes an issue in the Snapshot and Restore UI, where editing a Snapshot Lifecycle Management policy may fail if it contains a previous snapshot failure with a large payload content.