-
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
[Stack monitoring] Remove angular #115063
[Stack monitoring] Remove angular #115063
Conversation
@elasticmachine merge upstream |
merge conflict between base and head |
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
@@ -9,37 +9,21 @@ import React from 'react'; | |||
import { render } from 'react-dom'; | |||
import { get, includes } from 'lodash'; | |||
import { i18n } from '@kbn/i18n'; | |||
import { HttpStart, IHttpFetchError } from 'kibana/public'; |
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.
Changes in this file are because x-pack/plugins/monitoring/public/application/setup_mode/setup_mode.tsx
has been moved here
I added @phillipb and @neptunian as reviewers as well. I think it would be good to get a few eyeballs on this one. |
@elasticmachine merge upstream |
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.
Seems to work pretty well overall. I had a couple of comments on stuff that wasn't just wholesale removal.
Also, should we remove the render_react_app
config definition or do you plan to do that as a followup PR?
@@ -73,7 +73,7 @@ export const AlertsBadge: React.FC<Props> = (props: Props) => { | |||
const groupByType = GROUP_BY_NODE; | |||
const panels = showByNode | |||
? getAlertPanelsByNode(PANEL_TITLE, alerts, stateFilter) | |||
: getAlertPanelsByCategory(PANEL_TITLE, inSetupMode, alerts, stateFilter); | |||
: getAlertPanelsByCategory(PANEL_TITLE, !!inSetupMode, alerts, stateFilter); |
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.
Curious when this wouldn't be a boolean that required the !!
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.
It's because of this line. In React, we default to the isInSetupMode
that it's in the global state, that is marked as optional.
Should it be optional? Probably no, but I guess because of the progressive migration it didn't exist until we migrated the setup mode related stuff, so when we put it in the global state we put it as optional to not break anything that already exists.
Since, in this case, when it's undefined
, we should treat it as false
, I think is good enough to make the conversion here.
@@ -83,7 +83,7 @@ function waitForSetupModeData() { | |||
return new Promise((resolve) => process.nextTick(resolve)); | |||
} | |||
|
|||
describe('setup_mode', () => { | |||
xdescribe('setup_mode', () => { |
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.
.skip seems to be more common (I had to google what the x prefix did)
xdescribe('setup_mode', () => { | |
describe.skip('setup_mode', () => { |
Should we have a comment on why we're skipping it?
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 don't think is needed, It's already a task in the parent task #109791, and my plan was to fix them as soon as this PR is closed, together with all the other clean up tasks.
@@ -18516,7 +18514,6 @@ | |||
"xpack.monitoring.logstash.node.advanced.pageTitle": "Logstash 节点:{nodeName}", | |||
"xpack.monitoring.logstash.node.advanced.routeTitle": "Logstash - {nodeName} - 高级", | |||
"xpack.monitoring.logstash.node.pageTitle": "Logstash 节点:{nodeName}", | |||
"xpack.monitoring.logstash.node.pipelines.notAvailableDescription": "仅 Logstash 版本 6.0.0 或更高版本提供管道监测功能。此节点正在运行版本 {logstashVersion}。", |
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 appears to be just a translation cleanup. Unless it helps the angular removal cause I'd be inclined to do this in a separate PR.
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.
If we don't do the clean-up one step of the ci fails because these strings don't exist anymore.
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.
CI is already passing but running node scripts/i18n_check --fix
will update this file automatically
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/ml/stack_management_jobs/manage_spaces·ts.machine learning stack management jobs manage spaces assign to all spaces should display original job only in assigned spacesStandard Out
Stack Trace
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
It's also part of the follow-up. I tried to do only the essential here as a first step. |
@matschaffer if I'm not missing anything, all these issues are not related to deleting the code, these are bugs that already existed in React. For example, I also noticed the Logstash breadcrumbs and opened an issue some days ago #114807 In my opinion, I think is better if we do the same for each one of the bugs you found to fix them in separate PRs. It would help also in prioritizing them. |
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.
LGTM
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.
all these issues are not related to deleting the code, these are bugs that already existed in React.
Agreed.
Was noting them for posterity (like turning into discrete issues), not things I'd consider blocking merge. Let's do this thing.
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.
LGTM
* Remove angular * Fix translations * convert insetupmode to boolean * remove license service Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
* Remove angular * Fix translations * convert insetupmode to boolean * remove license service Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Ester Martí Vilaseca <[email protected]>
Summary
Closes #115207 by removing Angular and the basic files of the Angular code.
setup_mode.tsx
NOTE
The parent issue contains a list of tasks the will be done in follow-up PRs.