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

[Watcher] Move out of legacy #54752

Merged
merged 21 commits into from
Jan 23, 2020

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Jan 14, 2020

Summary

Continuation of work started in #50908. This is the final step of the New Platform (NP) migration.

TODOs before review

  • Figure out what to do with TimeBuckets and MANAGEMENT_SECTION dependencies. Currently this lives inside of a folder called legacy in the watcher NP folder.
  • Figure out how we can work around the requirement of style sheets for now (there are a smaller number of styles in watcher, but they should be ported over too)
  • More testing on server side, how do we handle un-registering routes? Does the app restart both client and server side?
  • Fix tests and CI

How to Review

  • I would recommend looking mainly at public/plugin.ts and server/plugin.ts. Most important changes are there - how the plugin starts up and the services it uses.
  • For TimeBucket dep we copied the code to the watcher plugin (per [ML] Copy over TimeBuckets into ML code #44249)
  • Could be helpful to have a second local copy of the kibana repo so that you can use git-diff like git diff --no-index ../current-master/watcher/public/np_ready/application ./x-pack/plugins/watcher/public/application for a cleaner summary of application specific changes
    • Removed the license expired state as the plugin will not render under those conditions at all now
    • Fixed a router warning issued from license_management, we can move this to another PR but it's quite a small fix
    • legacy folder contains TimeBucket and MANAGEMENT_BREADCRUMB.
  • Otherwise, run through UI and ensure that nothing looks weird, create a threshold watch and a JSON watch and let them fire.

TODO

  • Revert removal of license error placeholder for now

@jloleysens jloleysens added Feature:Watcher v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.7.0 roadmap labels Jan 14, 2020
@elasticmachine
Copy link
Contributor

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

@jloleysens
Copy link
Contributor Author

@joshdover

There are two issues I'm noticing as I was working on this:

1

I'm seeing a situation with watcher when I am on a license that permits use of watcher (e.g., platinum) then run:

curl -XDELETE -v http://elastic:changeme@localhost:9200/_license
curl -XPOST -v http://elastic:changeme@localhost:9200/_license/start_basic\?acknowledge\=true

which reverts to basic, then do a page refresh my links still renders. Clicking on it then hides it as this causes a re-render.

2

The functionality in src/core/public/chrome/ui/header/header.tsx (extendNavLink I think) can get lead into a situation where if the user was on a page in management (like watcher) that is now disabled (as above), they won't be able to get back into management because the app is no longer registered and current default behaviour is to go back to Kibana home, not management home. Perhaps not something new :) just wondering if this is worth creating an issue.

@jloleysens jloleysens marked this pull request as ready for review January 16, 2020 14:54
@jloleysens jloleysens added the release_note:skip Skip the PR/issue when compiling release notes label Jan 16, 2020
@joshdover
Copy link
Contributor

1

I'm seeing a situation with watcher when I am on a license that permits use of watcher (e.g., platinum) then run:

curl -XDELETE -v http://elastic:changeme@localhost:9200/_license
curl -XPOST -v http://elastic:changeme@localhost:9200/_license/start_basic\?acknowledge\=true

which reverts to basic, then do a page refresh my links still renders. Clicking on it then hides it as this causes a re-render.

It only does this when you revert from trial to basic, or also when starting with basic?

And by link, you mean the management section link, correct?

If you put logs in your license check code, are the logs also broken? This would be helpful in narrowing down whether or not this is a UI issue or an issue with the licensing plugin.

2

The functionality in src/core/public/chrome/ui/header/header.tsx (extendNavLink I think) can get lead into a situation where if the user was on a page in management (like watcher) that is now disabled (as above), they won't be able to get back into management because the app is no longer registered and current default behaviour is to go back to Kibana home, not management home. Perhaps not something new :) just wondering if this is worth creating an issue.

I'm not sure if that is new behavior either, but if so, that would be an issue that maybe the @mattkime could help with. I know he's been digging in the Manaagement app recently for the migration to the New Platform.

@joshdover
Copy link
Contributor

joshdover commented Jan 16, 2020

Regarding problem (1):

After reading the code, I think this problems stems from the code around trying to conditionally register the Watcher section based on licensing updates and what not.

I don't think this is the right approach (though the only possible one right now). Instead, I think the Management API needs to support a similar updater$ API to the Application Service (#50223). This would allow plugins to toggle their management app's visibility in the Management navigation based on licensing, feature controls, or other factors. @mattkime what do you think?

if (this.hasValidLicense) {
const esSection = management.sections.getSection('elasticsearch');
if (esSection) {
if (!this.hasRegisteredESManagementSection) {
Copy link
Contributor

Choose a reason for hiding this comment

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

All of this logic could be great simplified if an API like this existed, similar to the Application API:

esSection.registerApp({
  id: 'watcher',
  title: 'Watcher',
  updater$: licensing.license$.pipe(map(license => {
    const { state } = license.check(PLUGIN.ID, PLUGIN.MINIMUM_LICENSE_REQUIRED);
    const hasValidLicense =
      state === LICENSE_CHECK_STATE.Valid && license.getFeature(PLUGIN.ID).isAvailable;
    return {
      status: hasValidLicense ? ManagementAppStatus.accessible : ManagementAppStatus.inaccessible
    };
  })),
  mount(...) { ... },
});

@jloleysens
Copy link
Contributor Author

@joshdover , for further clarification on scenario 1:

Console logs:
Screenshot 2020-01-17 at 13 34 30

Code changes to emit log:
Screenshot 2020-01-17 at 13 34 42

This speaks to what you've pointed out.

It's specific to the scenario where I've issued a request directly against ES (not from my browser session) to revert to basic. I suppose this would also happen if another user changed the license while I was in Kibana and there was a second emission on the license$. So even though we've disabled the app and prevented it from rendering at all, the management side menu still renders Watcher.

For the second issue I was encountering, I've opened this issue (not sure if there already is one): #55170

…t-of-legacy

* 'master' of github.com:elastic/kibana: (142 commits)
  [Vis] Move Timelion Vis to vis_type_timelion (elastic#52069)
  Deprecate `chrome.navlinks.update` and add documentation (elastic#54893)
  [ML] Single Metric Viewer: Fix time bounds with custom strings. (elastic#55045)
  [Vis: Default editor] EUIficate and Reactify the sidebar (elastic#49864)
  [Mappings editor] Fix cannot set boolean value for "null_value" param (elastic#55015)
  [SIEM] Adds support for apm-* to the network map (elastic#54876)
  [Reporting] Define shims of legacy dependencies (elastic#54082)
  Resolver is overflow: hidden to prevent obscured elements from showing up (elastic#55076)
  Upgraded EUI to 18.2.1 (elastic#55090)
  [Maps] Support styles on agg fields with _of_ in name (elastic#54965)
  Remove xpack_main requirement, it's no longer in use (elastic#55060)
  Fix Snapshots Policies Alignment Issue in IE11 (elastic#54866)
  first rule cuts (elastic#54990)
  [DOCS] Adds geocentroid note to coordinate map (elastic#54389)
  [Canvas] Fixes the Copy Post Url link (elastic#54831)
  Fixes bugs with full screen filters (elastic#54792)
  [ML] Fix decoding in the URL state  (elastic#54915)
  Remove redundant `x-pack/typings`. (elastic#55042)
  [SIEM][Detection Engine] Adds critical missing status route to prepackaged rules
  Generate legacy vars when rendering all applications (elastic#54768)
  ...

# Conflicts:
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Thanks for the orderly commit history! I was able to follow along pretty well. Code LGTM. I've run out of time today so if you'd like me to test locally just hold off on merging and I will get to it tomorrow. Otherwise feel free to merge if you're confident there are no regressions. Might be worth asking @cuff-links to QA this after 7.6 is released.


setup(
{ application, notifications, http, uiSettings, getStartServices }: CoreSetup,
{ licensing, management, data, home }: Dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't dug around for this, so apologies for the dumb question, but what's the data dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like a collection of data access functions (based on src/plugins/data/README.md) 😂. There is functionality in there that was pulled from legacy in the TimeBuckets which have been included here.

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

elasticmachine and others added 3 commits January 22, 2020 05:40
…t-of-legacy

* 'master' of github.com:elastic/kibana:
  Restore 200 OK + response payload for save and delete endpoints (elastic#55535)
  [SIEM] [Detection Engine] Log time gaps as failures for now (elastic#55515)
  [NP] KibanaRequest provides request abortion event (elastic#55061)
…t-of-legacy

* 'master' of github.com:elastic/kibana:
  [Watcher] Add support for additional watch action statuses (elastic#55092)
  [ML] Fix entity controls update. (elastic#55524)
  [ML] Fixing ML's watcher integration (elastic#55439)
  [ML] Fixes permissions checks for data visualizer create job links (elastic#55431)
  [SearchProfiler] Move out of legacy (elastic#55331)

# Conflicts:
#	x-pack/plugins/watcher/server/models/action_status/action_status.js
@cjcenizal
Copy link
Contributor

Tested locally by creating, editing, and deleting both threshold and JSON watches. I also verified that the request flyouts, execution histories, and activation/deactivation actions all work as expected.

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@jloleysens jloleysens merged commit bb37b0f into elastic:master Jan 23, 2020
@jloleysens jloleysens deleted the np/watcher/out-of-legacy branch January 23, 2020 14:35
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 23, 2020
* Moved out of legacy folder

* First iteration of watcher plugin that renders

* Move create Timebuckets to plugin root
Update route registration and fix license checking for NP

* Re-enable Component integration tests

* Minor fix for data deserializer in api.ts

* Slight logic refactor, more defensive plugin startup

* Re-add legacy folder for SCSS pipeline

* Remove duplicate style sheet

* Fix type issue with TimeBuckets export

* Update license management routing logic (issued warning for using basepath on navigating away from license management)
Remove commented out code in watcher

* More defensive plugin registration

* Fix i18n issues and restore registration of feature on home view

* Remove watcher license error check copy

* Restore license error message in watcher

* Fix mock context value

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

# Conflicts:
#	.github/CODEOWNERS
#	x-pack/legacy/plugins/watcher/public/7_x_only.ts
#	x-pack/plugins/watcher/public/application/boot.tsx
jloleysens added a commit that referenced this pull request Jan 23, 2020
* Moved out of legacy folder

* First iteration of watcher plugin that renders

* Move create Timebuckets to plugin root
Update route registration and fix license checking for NP

* Re-enable Component integration tests

* Minor fix for data deserializer in api.ts

* Slight logic refactor, more defensive plugin startup

* Re-add legacy folder for SCSS pipeline

* Remove duplicate style sheet

* Fix type issue with TimeBuckets export

* Update license management routing logic (issued warning for using basepath on navigating away from license management)
Remove commented out code in watcher

* More defensive plugin registration

* Fix i18n issues and restore registration of feature on home view

* Remove watcher license error check copy

* Restore license error message in watcher

* Fix mock context value

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

# Conflicts:
#	.github/CODEOWNERS
#	x-pack/legacy/plugins/watcher/public/7_x_only.ts
#	x-pack/plugins/watcher/public/application/boot.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration Feature:Watcher release_note:skip Skip the PR/issue when compiling release notes roadmap Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants