-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[ML] Kibana management jobs list #42570
[ML] Kibana management jobs list #42570
Conversation
Pinging @elastic/ml-ui |
f8fc074
to
0d1cf35
Compare
💚 Build Succeeded |
💔 Build Failed |
💚 Build Succeeded |
💔 Build Failed |
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.
Overall LGTM, great job! Just added a few comments/questions.
x-pack/legacy/plugins/ml/public/jobs/jobs_list/components/job_details/job_details.js
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/ml/public/jobs/jobs_list/components/job_group/job_group.js
Show resolved
Hide resolved
x-pack/legacy/plugins/ml/public/jobs/jobs_list/components/jobs_list/jobs_list.js
Show resolved
Hide resolved
💚 Build Succeeded |
💔 Build Failed |
672dc02
to
7b1d091
Compare
privileges = capabilities; | ||
// Loop through all privilages to ensure they are all set to true. | ||
const isManageML = Object.keys(privileges).every( | ||
privilegeType => privileges[privilegeType as PrivilageKey] === true |
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 is a nice typescripty solution to get round the strictness of looping over interface keys, but it could be avoided by using Object.values
const isManageML = Object.values(privileges).every(p => p === true);
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.
Updated in 9d707f9
💚 Build Succeeded |
]; | ||
|
||
if (mlAnnotationsEnabled) { | ||
if (fullDetails) { |
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.
nit, it's not immediately clear what fullDetails
is, especially when used in a truthy check like this.
perhaps showFullDetails
would be clearer.
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.
isManagementTable
is used later on, perhaps it should also be used here?
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.
Updated to showFullDetails
- 9d707f9
x-pack/legacy/plugins/ml/public/jobs/jobs_list/components/job_filter_bar/job_filter_bar.js
Show resolved
Hide resolved
x-pack/legacy/plugins/ml/public/jobs/jobs_list/components/job_filter_bar/job_filter_bar.js
Show resolved
Hide resolved
// Adding 'width' props to columns for use in the Kibana management jobs list table | ||
// The version of the table used in ML > Job Managment depends on many EUI class overrides that set the width explicitly. | ||
// The ML > Job Managment table won't change as the overwritten class styles take precedence, though these values may need to | ||
// be updated if we move to always using props for width. |
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.
css overrides where originally used because it wasn't possible to add widths to the column objects.
it would be good to remove the css overrides and have all widths here.
This could be done in a follow up PR
jobId | ||
}; | ||
const encoded = rison.encode(settings); | ||
const url = `?mlManagement=${encoded}`; |
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.
is mlManagement
needed in the URL?
the jobId
may be enough.
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 wanted it to be explicit and it gets removed from the url as soon as the filter is cleared.
@@ -75,6 +79,14 @@ class JobsListUI extends Component { | |||
this.props.toggleRow(item.id); | |||
}; | |||
|
|||
getJobIdLink(id) { |
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.
Is it possible to disable the links to the Jobs list (inside ML) and the results views if ML is not enabled in the space? Currently in that situation you just get the page with the {"statusCode":404,"error":"Not Found","message":"Not Found"}
error message
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.
Added as TODO in a follow-up 👍
EuiTitle, | ||
} from '@elastic/eui'; | ||
|
||
export const AccessDeniedPage = () => ( |
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 page shows up if the user is on a basic license. Just checking if the ML management menu should be hidden altogether?
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.
from the discussions we had, the expected behaviour was that if the licence and feature checks fail, the ML section would be disabled altogether.
this is checked using features.ml.showLinks
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.
Good catch - updated in 9d707f9
}, { | ||
name: intl.formatMessage({ |
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.
As above, is it possible to hide this column if ML is not enabled in the space?
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.
Added as a TODO in a follow-up 👍
@@ -7,6 +7,8 @@ | |||
import { each } from 'lodash'; | |||
import { toastNotifications } from 'ui/notify'; | |||
import { mlMessageBarService } from 'plugins/ml/components/messagebar/messagebar_service'; | |||
import rison from 'rison-node'; | |||
import chrome from 'ui/chrome'; // TODO: get from context once walter's PR is merged |
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.
Can you use the new use_ui_chrome_context
custom hook here?
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { management } from 'ui/management'; |
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.
Could this new file be TypeScript?
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.
Updated in 9d707f9
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
💚 Build Succeeded |
…p-metrics-selectall * 'master' of github.com:elastic/kibana: (22 commits) [Code]: downgrade the log level of error message from subprocess (elastic#42925) [Code] Cancel clone/update job in the middle if disk space over the watermark (elastic#42890) Add Kibana App specific URL to the help menu (elastic#34739) (elastic#42580) [Maps] refactor createShapeFilterWithMeta to support more than just polygons (elastic#43042) Skip flaky es_ui_shared/request tests. Pass uiSettings to all data plugin services (elastic#42159) [SIEM] Upgrades react-redux and utilize React.memo for performance gains (elastic#43029) [skip-ci][Maps] add maki icon sheet to docs (elastic#43063) Adding "style-src 'unsafe-inline' 'self'" to default CSP rules (elastic#41305) Update dependency commander to v3 (elastic#43041) Update dependency @percy/agent to ^0.10.0 (elastic#40517) [Maps] only show top hits checkbox if index has date fields (elastic#43056) run chained_controls on Firefox to catch regression (elastic#43044) fixing issue with dashboard csv download (elastic#42964) Expose task manager as plugin instead of server argument (elastic#42966) Expose createRouter from HttpService, prepare handlers for context introduction (elastic#42686) [Code] disk watermark supports percentage and absolute modes (elastic#42987) [apps/dashboard] skip part of filtering tests on FF (elastic#43047) [ML] Kibana management jobs list (elastic#42570) [ML] Fix check for watcher being enabled (elastic#43025) ...
Summary
In preparation for space-aware jobs, provides a place where users can view all jobs.
ML
section andJobs list
subsection to Kibana management page.Expanded row
View if required permissions are not met:
To be added in follow-up PRs
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] Documentation was added for features that require explanation or tutorials- [ ] Unit or functional tests were updated or added to match the most common scenariosFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately