Skip to content

[Watcher] New Platform (NP) Migration - #50908

Merged
jloleysens merged 38 commits into
elastic:masterfrom
jloleysens:np-migration/watcher
Dec 11, 2019
Merged

jloleysens merged 38 commits into
elastic:masterfrom
jloleysens:np-migration/watcher

Conversation

@jloleysens

@jloleysens jloleysens commented Nov 18, 2019 •

Copy link
Copy Markdown
Contributor

Summary

NP migration for the watcher application.

Public

Public is a large number of dependencies and should be properly regression tested (TODO). Most tests expect React ^16.9.

Server

Not many changes here at the moment, mostly consumes server.route and server.plugins.elasticsearch from legacy.

How to Reiview

Public

  • All plugin dependencies or state derived from plugin dependencies have been moved into a new app wide context that was created.
  • Cleaned up use of React hooks to be in line with Eslint rules. In two points it was explicitly opted out of due to wrong behaviour:
    • x-pack/legacy/plugins/watcher/public/np_ready/application/sections/watch_edit/components/threshold_watch_edit/watch_visualization.tsx
    • x-pack/legacy/plugins/watcher/public/np_ready/application/sections/watch_edit/components/threshold_watch_edit/threshold_watch_edit.tsx
  • All watcher application files now live in x-pack/legacy/plugins/watcher/public/np_ready/application (so lots of path changes)
  • Everything inside models directory has been left as is (still JS)
  • Client tests are still disabled (they have been for some time, this just makes refactors like this more risky!). See __jest__ folder.
  • Minor changes to a shared dependency src/plugins/es_ui_shared/public/request/np_ready_request.ts to also support query in network requests.

Server

  • Migrated to NP router and from JS -> TS. This introduced a large number of changes inside of x-pack/legacy/plugins/watcher/server/np_ready/routes/api. Only manual testing performed via the UI, no pre-existing tests! There are some functional tests that help here, but still needed to do a large amount of manual testing
  • models has been left untouched
  • Migrated to new featureCatalogue APIs
  • Migrated all of lib to TS.
  • Removed wrap_es_errors from lib - we are just using new kibana response directly.

Deferred

  • looks like there is an issue where when a watch fires and you ack it, then switch to history and back to status, the stale unacked version shows up. Will create an issue after this PR is merged.

Still need to switch to np ready version of use_request
…her [skip ci]

* upstream/master: (54 commits)
  allows plugins to define validation schema for "enabled" flag (elastic#50286)
  Add retry to find.existsByDisplayedByCssSelector (elastic#48734)
  [i18n] integrate latest translations (elastic#50864)
  ui/resize_checker 👉 src/plugins/kibana_utils (elastic#44750)
  Fix @reach/router types (elastic#50863)
  [ML] Adding ML node warning to overview and analytics pages (elastic#50766)
  Bump storybook dependencies (elastic#50752)
  [APM Replace usage of idx with optional chaining (elastic#50849)
  [SIEM] Fix eslint errors (elastic#49713)
  Improve "Browser client is out of date" error message (elastic#50296)
  [SIEM][Detection Engine] REST API improvements and changes from UI/UX feedback (elastic#50797)
  Move @kbn/es-query into data plugin - es-query folder (elastic#50182)
  Index Management new platform migration (elastic#49359)
  Increase retry for cloud snapshot to finish (elastic#50781)
  Removing EuiCode from inside EuiPanel (elastic#50683)
  [SIEM] Tests for search_after and bulk index (elastic#50129)
  Make babel understand TypeScript 3.7 syntax (elastic#50772)
  Fixing mocha tests and broken password change status codes (elastic#50704)
  [Canvas] Use compressed forms in sidebar (elastic#49419)
  Add labels to shell scripts in Jenkins (elastic#49657)
  ...
- Some updates after API changes
@jloleysens jloleysens added release_note:roadmap Feature:Watcher v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// v7.6.0 labels Nov 18, 2019
@jloleysens
jloleysens requested a review from rudolf November 18, 2019 14:31
@elasticmachine

Copy link
Copy Markdown
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@jloleysens

Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@jloleysens

Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@jloleysens

jloleysens commented Dec 10, 2019 •

Copy link
Copy Markdown
Contributor Author

@cjcenizal Thanks for the review! :D

@alisonelizabeth Thanks for the review too! :D

In response to your comments/questions:

I think it would be great to remove the skip on the component integration tests...

I think this can be considered a separate piece of work as it's more related to the recent react 16.12 upgrade. I created this issue #52620 for tracking this work - I am happy to tackle this straight after :). I get that getting test coverage after a refactor like this is really valuable! Fwiw, after unskipping thewatch_create_json.test.tsx I was seeing the following kind of errors:

    Expected: "{\"executeDetails\":{\"triggerData\":{\"triggeredTime\":\"now+5s\",\"scheduledTime\":\"now+5s\"},\"ignoreCondition\":true,\"actionModes\":{\"my-logging-action\":\"force_execute\"}},\"watch\":{\"id\":\"my-test-watch\",\"type\":\"json\",\"isNew\":true,\"actions\":[],\"watch\":{\"trigger\":{\"schedule\":{\"interval\":\"30m\"}},\"input\":{\"search\":{\"request\":{\"body\":{\"size\":0,\"query\":{\"match_all\":{}}},\"indices\":[\"*\"]}}},\"condition\":{\"compare\":{\"ctx.payload.hits.total\":{\"gte\":10}}},\"actions\":{\"my-logging-action\":{\"logging\":{\"text\":\"There are {{ctx.payload.hits.total}} documents in your index. Threshold is 10.\"}}}}}}"
    Received: "{\"body\":\"{\\\"executeDetails\\\":{\\\"triggerData\\\":{\\\"triggeredTime\\\":\\\"now+5s\\\",\\\"scheduledTime\\\":\\\"now+5s\\\"},\\\"ignoreCondition\\\":true,\\\"actionModes\\\":{\\\"my-logging-action\\\":\\\"force_execute\\\"}},\\\"watch\\\":{\\\"id\\\":\\\"my-test-watch\\\",\\\"type\\\":\\\"json\\\",\\\"isNew\\\":true,\\\"actions\\\":[],\\\"watch\\\":{\\\"trigger\\\":{\\\"schedule\\\":{\\\"interval\\\":\\\"30m\\\"}},\\\"input\\\":{\\\"search\\\":{\\\"request\\\":{\\\"body\\\":{\\\"size\\\":0,\\\"query\\\":{\\\"match_all\\\":{}}},\\\"indices\\\":[\\\"*\\\"]}}},\\\"condition\\\":{\\\"compare\\\":{\\\"ctx.payload.hits.total\\\":{\\\"gte\\\":10}}},\\\"actions\\\":{\\\"my-logging-action\\\":{\\\"logging\\\":{\\\"text\\\":\\\"There are {{ctx.payload.hits.total}} documents in your index. Threshold is 10.\\\"}}}}}}\"}"

Not sure they are related to the NP migration?

[Update]
Since the issues were caused by the NP migration to http we decided to unskip the tests here

What benefit do we get from creating the np_ready directory?

The primary benefit is outlined here in the migration guide. Basically linting for ensuring that we have moved direct use of all legacy ui/ deps out of the application logic. Also, ideally, the paths in application should mostly remain unchanged as we want to only move the stuff inside np_ready once we migrate :)

I noticed there's a lot of @ts-ignore scattered throughout the code.

Most of the remaining untyped JS modules are inside of models dirs. public has them any'ed through an index.d.ts file which uses the public/.. path convention for public code. So no matter from where they're used the module declaration requires only one entry in the index.d.ts file. I'm not sure the same exists for server side :( ? So everytime we use a model we need to append the unique path for the module to the index.d.ts file which is not that much easier than adding // @ts-ignore at this point. Overall I think the very best would be to convert them to TS which is quite a big job that I'd prefer we tackle as a separate PR 😆 . Wdyt? (there may be another way of solving this that I'm unaware of)

@alisonelizabeth

alisonelizabeth commented Dec 10, 2019 •

Copy link
Copy Markdown
Contributor

Thanks for the explanations, @jloleysens!

Fwiw, after unskipping thewatch_create_json.test.tsx I was seeing the following kind of errors:

Looks like it might be related. The "received" value is wrapped in body and is a JSON string. Otherwise, the content itself looks the same. I think this is a result of moving to the NP http service.

Most of the remaining untyped JS modules are inside of models dirs. public has them any'ed through an index.d.ts file which uses the public/.. path convention for public code. So no matter from where they're used the module declaration requires only one entry in the index.d.ts file. I'm not sure the same exists for server side :( ? So everytime we use a model we need to append the unique path for the module to the index.d.ts file which is not that much easier than adding // @ts-ignore at this point. Overall I think the very best would be to convert them to TS which is quite a big job that I'd prefer we tackle as a separate PR 😆 . Wdyt? (there may be another way of solving this that I'm unaware of)

Yeah, it will be quite a lot of work to convert all the models to TS. I think it's ok to leave as is for now - thanks!

@alisonelizabeth alisonelizabeth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested with the latest changes. LGTM. Thanks!

@cuff-links

cuff-links commented Dec 11, 2019 via email

Copy link
Copy Markdown
Contributor

@elasticmachine

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

History

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

@jloleysens
jloleysens merged commit aa31b53 into elastic:master Dec 11, 2019
@jloleysens
jloleysens deleted the np-migration/watcher branch December 11, 2019 08:54
@jloleysens

Copy link
Copy Markdown
Contributor Author

I want to run through the test suite before merging this. I will test it locally.
…
On Tue, Dec 10, 2019, 6:48 PM Elastic Machine @.***> wrote: 💚 Build Succeeded - continuous-integration/kibana-ci/pull-request https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/14677/ - Commit: 471c801 <471c801> History - 💚 Build #14585 https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/14585/ succeeded 6e90b77 <6e90b77> - 💚 Build #14515 https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/14515/ succeeded 61078aa <61078aa> - 💚 Build #14280 https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/14280/ succeeded cbf30e1 <cbf30e1> - 💔 Build #14262 https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/14262/ failed f65a526 <f65a526> - 💚 Build #14194 https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/14194/ succeeded 0ed3d4b <0ed3d4b> To update your PR or re-run it, just comment with: @elasticmachine merge upstream — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#50908?email_source=notifications&email_token=AAK5UDXUNXZ4QU5777EJCW3QYATGTA5CNFSM4JOUL47KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGRLPRY#issuecomment-564312007>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAK5UDWN4XDQPNOOHC5BWULQYATGTANCNFSM4JOUL47A .

Hey @cuff-links , sorry for merging - I missed this message. For any issues you've found I'll open up a fix PR 👍

jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 11, 2019
* First iteration of watch public -> new platform
Still need to switch to np ready version of use_request

* - Switched to using np ready request
- Some updates after API changes

* First attempt at server shim

* Rename file and re-enable react hooks linting

* Fix some public types and react hooks lint rules

* Fix types

* More ES lint react hooks fixes

* Migrated server lib -> ts. Part way done with migrating routes to NP router and TS

* Big subset of routes to TS and NP router - almost there

* Delete legacy error wrappers and moved last set of routes to TS and NP router

* Remove @ts-ignore's and update route registration to use shim with http router

* Added routes validations, fixes for hooks and fixes for types

* Fix more types and finish testing API routes

* Fix usage of feature catalogue and fix time buckets types

* Fix error message shape [skip ci]

* Split legacy from new platform dependencies server-side

* Refactor: Seperate client legacy and NP dependencies

* Add file: added types file

* Fix UISettings client type import

* Update license pre-routing factory spec

* Update variable names, use of I18nContext (use NP) and docs

* Use NP elasticsearchclient

* Simplify is_es_error_factory

* Fix types

* Improve code legibility and remove second use of `useAppContext`

* Use @kbn/config-schema (not validate: false) on routes!

* Fix watch create JSON spec

* Create threshold test working

* Unskip watch_edit.test.ts

* Unskip watch_list.test.ts

* Done re-enabling component integration tests

* TimeBuckets typo + remove unnecessary // @ts-ignore
jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 11, 2019
…le-sql-highlighting

* 'master' of github.com:elastic/kibana: (56 commits)
  Migrate url shortener service (elastic#50896)
  Re-enable datemath in from/to canvas timelion args (elastic#52159)
  [Logs + Metrics UI] Remove eslint exceptions (elastic#50979)
  [Logs + Metrics UI] Add missing headers in Logs & metrics (elastic#52405)
  [ML] API integration tests - initial tests for bucket span estimator (elastic#52636)
  [Watcher] New Platform (NP) Migration (elastic#50908)
  Decouple Authorization subsystem from Legacy API. (elastic#52638)
  [APM] Fix some warnings logged in APM tests (elastic#52487)
  [ui/public/utils] Delete unused base_object & find_by_param (elastic#52500)
  [ui/public/utils] Move items into ui/vis (elastic#52615)
  fix newlines in kbn-analytics build script
  Add top level examples folder and command to run, `--run-examples`. (elastic#52027)
  feat(NA): add trap for SIGINT in the git precommit hook (elastic#52662)
  [DOCS] Updtes description of elasticsearch.requestHeadersWhitelist (elastic#52675)
  [Telemetry/Pulse] Updates advanced settings text for usage data (elastic#52657)
  [SIEM][Detection Engine] Adds the default name space to the end of the signals index
  [Logs UI] Generalize ML module management (elastic#50662)
  Removing stateful saved object finder (elastic#52166)
  Shim oss telemetry (elastic#51168)
  [Reporting/Screenshots] Do not fail the report if request is aborted (elastic#52344)
  ...

# Conflicts:
#	src/legacy/core_plugins/console/public/legacy.ts
#	src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/mode/elasticsearch_sql_highlight_rules.ts
#	src/legacy/core_plugins/console/public/np_ready/lib/autocomplete/components/full_request_component.ts
#	src/legacy/core_plugins/console/public/quarantined/src/sense_editor/row_parser.js
jloleysens added a commit that referenced this pull request Dec 11, 2019
* [Watcher] New Platform (NP) Migration (#50908)

* First iteration of watch public -> new platform
Still need to switch to np ready version of use_request

* - Switched to using np ready request
- Some updates after API changes

* First attempt at server shim

* Rename file and re-enable react hooks linting

* Fix some public types and react hooks lint rules

* Fix types

* More ES lint react hooks fixes

* Migrated server lib -> ts. Part way done with migrating routes to NP router and TS

* Big subset of routes to TS and NP router - almost there

* Delete legacy error wrappers and moved last set of routes to TS and NP router

* Remove @ts-ignore's and update route registration to use shim with http router

* Added routes validations, fixes for hooks and fixes for types

* Fix more types and finish testing API routes

* Fix usage of feature catalogue and fix time buckets types

* Fix error message shape [skip ci]

* Split legacy from new platform dependencies server-side

* Refactor: Seperate client legacy and NP dependencies

* Add file: added types file

* Fix UISettings client type import

* Update license pre-routing factory spec

* Update variable names, use of I18nContext (use NP) and docs

* Use NP elasticsearchclient

* Simplify is_es_error_factory

* Fix types

* Improve code legibility and remove second use of `useAppContext`

* Use @kbn/config-schema (not validate: false) on routes!

* Fix watch create JSON spec

* Create threshold test working

* Unskip watch_edit.test.ts

* Unskip watch_list.test.ts

* Done re-enabling component integration tests

* TimeBuckets typo + remove unnecessary // @ts-ignore

* Backport for "xPack:defaultAdminEmail" 7.x specific behaviour
@jloleysens jloleysens mentioned this pull request Jan 14, 2020
5 tasks done
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
* First iteration of watch public -> new platform
Still need to switch to np ready version of use_request

* - Switched to using np ready request
- Some updates after API changes

* First attempt at server shim

* Rename file and re-enable react hooks linting

* Fix some public types and react hooks lint rules

* Fix types

* More ES lint react hooks fixes

* Migrated server lib -> ts. Part way done with migrating routes to NP router and TS

* Big subset of routes to TS and NP router - almost there

* Delete legacy error wrappers and moved last set of routes to TS and NP router

* Remove @ts-ignore's and update route registration to use shim with http router

* Added routes validations, fixes for hooks and fixes for types

* Fix more types and finish testing API routes

* Fix usage of feature catalogue and fix time buckets types

* Fix error message shape [skip ci]

* Split legacy from new platform dependencies server-side

* Refactor: Seperate client legacy and NP dependencies

* Add file: added types file

* Fix UISettings client type import

* Update license pre-routing factory spec

* Update variable names, use of I18nContext (use NP) and docs

* Use NP elasticsearchclient

* Simplify is_es_error_factory

* Fix types

* Improve code legibility and remove second use of `useAppContext`

* Use @kbn/config-schema (not validate: false) on routes!

* Fix watch create JSON spec

* Create threshold test working

* Unskip watch_edit.test.ts

* Unskip watch_list.test.ts

* Done re-enabling component integration tests

* TimeBuckets typo + remove unnecessary // @ts-ignore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:NP Migration Feature:Watcher Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// v7.6.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants