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.

Sign up for GitHub

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

Jump to bottom

[Monitoring] NP migration: Local angular module #51823

Merged
merged 36 commits into from
Feb 10, 2020
Merged

[Monitoring] NP migration: Local angular module #51823

merged 36 commits into from
Feb 10, 2020

Conversation

igoristic
Copy link
Contributor

@igoristic igoristic commented Nov 27, 2019 •
edited
Loading

This PR includes

  • A completely isolated and local angular app
  • No longer dependents on ui/routes and ui/modules
  • All: services, filters, directives, routes, factories, and providers have been isolated from kibana module
  • globalState and timefilter isolated and shimmed

TODO:

@elasticmachine
Copy link
Contributor

💔 Build Failed

@mshustov
Copy link
Contributor

mshustov commented Dec 2, 2019

@igoristic are you going to merge this branch and work on TODO from the title in the following PRs? I'd recommend this way instead of a long-lived branch. We can start testing earlier.

@igoristic
Copy link
Contributor Author

@igoristic are you going to merge this branch and work on TODO from the title in the following PRs? I'd recommend this way instead of a long-lived branch. We can start testing earlier.

@restrry I agree 👍 I will first address your feedback, and also fix all the tests (and linting)

@igoristic igoristic mentioned this pull request Dec 5, 2019
Closed
8 tasks
@cachedout
Copy link
Contributor

Hi @igoristic. I am just checking on this. Is there anything blocking this besides incorporating the review feedback, the tests, and the linting?

@igoristic
Copy link
Contributor Author

Hi @igoristic. I am just checking on this. Is there anything blocking this besides incorporating the review feedback, the tests, and the linting?

@cachedout

No

@igoristic igoristic marked this pull request as ready for review December 6, 2019 13:38
@igoristic igoristic added the Team:Monitoring Stack Monitoring team label Dec 6, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring (Team:Monitoring)

@cachedout
Copy link
Contributor

Great job, @igoristic! I will look at this ASAP.

@igoristic igoristic requested a review from mshustov December 6, 2019 13:49
@elasticmachine
Copy link
Contributor

💔 Build Failed

cachedout
cachedout approved these changes Dec 9, 2019
View reviewed changes
Copy link
Contributor

@cachedout cachedout left a comment

Choose a reason for hiding this comment

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

I will leave a thorough review of the code changes up to others but I went through the functionality of the application with this PR and I couldn't find any issues.

chrisronline
chrisronline reviewed Dec 16, 2019
View reviewed changes
Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

I think this is looking great! Definitely a ton of work to get this all working and I'm super excited to start moving forward with this effort!

I left a few comments and will take another pass later this week

x-pack/legacy/plugins/monitoring/public/lib/setup_mode.test.js Outdated Show resolved Hide resolved
x-pack/legacy/plugins/monitoring/public/np_imports/angular/index.ts Show resolved Hide resolved
x-pack/legacy/plugins/monitoring/public/np_imports/angular/index.ts Show resolved Hide resolved
x-pack/legacy/plugins/monitoring/public/np_imports/angular/index.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/monitoring/public/np_imports/angular/modules.ts Outdated Show resolved Hide resolved
@igoristic
Copy link
Contributor Author

@chrisronline

If load the monitoring app through the monitoring icon on the left, the app works fine. However, if I refresh the page once I've loaded some monitoring page, the app fails to render.

Yeah, actually I was already aware of that bug, and thought I merged the branch that fixed it. But, since there was a conflict I guess it was ignored.

chrisronline
chrisronline approved these changes Feb 5, 2020
View reviewed changes
Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

Functionally, this LGTM! Great work! I don't know much about the NP migration guidelines, so I'll defer that to @restrry

@mshustov
Copy link
Contributor

mshustov commented Feb 5, 2020

ack: will check the problem tomorrow

@mshustov
Copy link
Contributor

mshustov commented Feb 6, 2020 •
edited
Loading

I didn't manage to reproduce the problem. how tested:

  • set monitoring.ui.enabled: true
  • landed in /app/monitoring
  • landed on /app/monitoring#/no-data?_g=(inSetupMode:!t)
  • landed on /app/monitoring#/elasticsearch/nodes?_g=(inSetupMode:!t)

mshustov
mshustov approved these changes Feb 7, 2020
View reviewed changes
x-pack/legacy/plugins/monitoring/index.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/monitoring/index.ts
},

postInit(server: Server) {
const { infra } = server.plugins as Partial<typeof server.plugins> & { infra?: InfraPlugin };
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't you declare dependency on infra plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean through kibana.json? We'll do that in the next phase when we move the client outside the legacy directory

Copy link
Contributor

Choose a reason for hiding this comment

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

not only, you declare dependencies on other plugins https://github.com/elastic/kibana/pull/51823/files#diff-3450f9bd06407cda6d6d9e26ddeb0e11R31-R35

x-pack/legacy/plugins/monitoring/index.ts
'monitoring.ui.elasticsearch.hosts',
'monitoring.ui.elasticsearch',
'monitoring.xpack_api_polling_frequency_millis',
'server.uuid',
Copy link
Contributor

@mshustov mshustov Feb 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

you can already start consuming them from server.newPlatform.setup.core.http.getServerInfo and server.newPlatform.setup.core.uuid.getInstanceUuid

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 think this will also be in the next phase, when we start using np alternatives and whitelisting config properties

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough

@igoristic
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

@igoristic igoristic merged commit 205c2ab into elastic:master Feb 10, 2020
@igoristic igoristic deleted the np_ready_2 branch February 10, 2020 20:57
@igoristic igoristic mentioned this pull request Feb 10, 2020
Merged
@igoristic igoristic removed the v7.6.1 label Feb 10, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 11, 2020
@gmmorris
Merge branch 'master' into alerting/alert-instances
91bd390 
* master: (34 commits)
  [Index management] Server-side NP ready (elastic#56829)
  Webhook action - make user and password secrets optional (elastic#56823)
  [DOCS] Removes reference to IRC (elastic#57245)
  [Monitoring] NP migration: Local angular module (elastic#51823)
  [SIEM] Adds ECS link to help menu (elastic#57104)
  Ensure http interceptors are shares across lifecycle methods (elastic#57150)
  [Remote clusters] Migrate server code out of legacy (elastic#56781)
  fixes render bug in alert list (elastic#57152)
  siem 7.6 updates (elastic#57169)
  Make the update alert API key API work when AAD is out of sync (elastic#56640)
  fix(NA): MaxListenersExceededWarning on getLoggerStream (elastic#57133)
  [Metrics UI] Setup commonly used time ranges in timepicker (elastic#56701)
  [Maps] set filter.meta.key to geoFieldName so query passes filterMatchesIndex when ignoreFilterIfFieldNotInIndex is true (elastic#56692)
  Create plugin mock for event log plugin (elastic#57048)
  fix ts error on master (elastic#57236)
  Don't create API key for disabled alerts when calling create API (elastic#57041)
  Fix enable and disable API to still work when AAD is out of sync (elastic#56634)
  [DOCS] Canvas embed objects (elastic#57156)
  Delete autocomplete namespace (elastic#57187)
  Security - Inject logout url (elastic#57201)
  ...
igoristic added a commit that referenced this pull request Feb 11, 2020
@igoristic @elasticmachine
[7.x] [Monitoring] NP migration: Local angular module (#51823) (#57269)
b58b1d2 
* resolve conflicts

* Update index.ts

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@igoristic
Copy link
Contributor Author

Backport:
7.x: b58b1d2

@igoristic igoristic mentioned this pull request Feb 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mshustov mshustov mshustov approved these changes

@cachedout cachedout cachedout approved these changes

@chrisronline chrisronline chrisronline approved these changes

Assignees
No one assigned
Labels
backported Feature:NP Migration release_note:enhancement Team:Monitoring Stack Monitoring team v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@igoristic @elasticmachine @mshustov @cachedout @kibanamachine @chrisronline