-
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
[ML] Space management UI #83320
[ML] Space management UI #83320
Changes from all commits
4c5bd53
ed43007
d03b434
105eb3d
1f74d8c
b4300cd
fa638c1
a3a7f39
78bc482
21a176c
8952cf3
29c6175
e90c091
e10663d
50cf87e
a6fd8ac
d42c2d1
b217713
ce92bdc
bcd05d3
9ed6cf6
37f041d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,20 +4,64 @@ | |
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import React, { FC } from 'react'; | ||
import React, { FC, useState, useEffect } from 'react'; | ||
|
||
import { EuiFlexGroup, EuiFlexItem, EuiBadge } from '@elastic/eui'; | ||
import { EuiFlexGroup, EuiFlexItem, EuiButtonEmpty } from '@elastic/eui'; | ||
import { JobSpacesFlyout } from '../job_spaces_selector'; | ||
import { JobType } from '../../../../common/types/saved_objects'; | ||
import { useSpacesContext } from '../../contexts/spaces'; | ||
import { Space, SpaceAvatar } from '../../../../../spaces/public'; | ||
|
||
export const ALL_SPACES_ID = '*'; | ||
|
||
interface Props { | ||
spaces: string[]; | ||
spaceIds: string[]; | ||
jobId: string; | ||
jobType: JobType; | ||
refresh(): void; | ||
} | ||
|
||
function filterUnknownSpaces(ids: string[]) { | ||
return ids.filter((id) => id !== '?'); | ||
} | ||
|
||
export const JobSpacesList: FC<Props> = ({ spaces }) => ( | ||
<EuiFlexGroup wrap responsive={false} gutterSize="xs"> | ||
{spaces.map((space) => ( | ||
<EuiFlexItem grow={false} key={space}> | ||
<EuiBadge color={'hollow'}>{space}</EuiBadge> | ||
</EuiFlexItem> | ||
))} | ||
</EuiFlexGroup> | ||
); | ||
export const JobSpacesList: FC<Props> = ({ spaceIds, jobId, jobType, refresh }) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
const { allSpaces } = useSpacesContext(); | ||
|
||
const [showFlyout, setShowFlyout] = useState(false); | ||
const [spaces, setSpaces] = useState<Space[]>([]); | ||
|
||
useEffect(() => { | ||
const tempSpaces = spaceIds.includes(ALL_SPACES_ID) | ||
? [{ id: ALL_SPACES_ID, name: ALL_SPACES_ID, disabledFeatures: [], color: '#DDD' }] | ||
: allSpaces.filter((s) => spaceIds.includes(s.id)); | ||
setSpaces(tempSpaces); | ||
}, [spaceIds, allSpaces]); | ||
|
||
function onClose() { | ||
setShowFlyout(false); | ||
refresh(); | ||
} | ||
|
||
return ( | ||
<> | ||
<EuiButtonEmpty onClick={() => setShowFlyout(true)} style={{ height: 'auto' }}> | ||
<EuiFlexGroup wrap responsive={false} gutterSize="xs"> | ||
{spaces.map((space) => ( | ||
<EuiFlexItem grow={false} key={space.id}> | ||
<SpaceAvatar space={space} size={'s'} /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Someone more familiar with this component should probably evaluate if this default (i.e., There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In most cases, the space avatar is rendered alongside a text description of the space, so we opted to make it "hidden" by default to avoid announcing it twice: #27748 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But if people don't know that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a fair point. Up until now, the security team have been the only consumers of this component, and its original design didn't expect broad usage at the time. It probably makes sense to revisit this decision now that it's being used in more places by more teams, who don't know of its history |
||
</EuiFlexItem> | ||
))} | ||
</EuiFlexGroup> | ||
</EuiButtonEmpty> | ||
{showFlyout && ( | ||
<JobSpacesFlyout | ||
jobId={jobId} | ||
spaceIds={filterUnknownSpaces(spaceIds)} | ||
jobType={jobType} | ||
onClose={onClose} | ||
/> | ||
)} | ||
</> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
export { JobSpacesRepairFlyout } from './job_spaces_repair_flyout'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,161 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import React, { FC, useState, useEffect } from 'react'; | ||
import { i18n } from '@kbn/i18n'; | ||
import { FormattedMessage } from '@kbn/i18n/react'; | ||
import { | ||
EuiFlyout, | ||
EuiFlyoutHeader, | ||
EuiFlyoutFooter, | ||
EuiFlexGroup, | ||
EuiFlexItem, | ||
EuiButton, | ||
EuiButtonEmpty, | ||
EuiTitle, | ||
EuiFlyoutBody, | ||
EuiText, | ||
EuiCallOut, | ||
EuiSpacer, | ||
} from '@elastic/eui'; | ||
|
||
import { ml } from '../../services/ml_api_service'; | ||
import { | ||
RepairSavedObjectResponse, | ||
SavedObjectResult, | ||
} from '../../../../common/types/saved_objects'; | ||
import { RepairList } from './repair_list'; | ||
import { useToastNotificationService } from '../../services/toast_notification_service'; | ||
|
||
interface Props { | ||
onClose: () => void; | ||
} | ||
export const JobSpacesRepairFlyout: FC<Props> = ({ onClose }) => { | ||
const { displayErrorToast, displaySuccessToast } = useToastNotificationService(); | ||
const [loading, setLoading] = useState(false); | ||
const [repairable, setRepairable] = useState(false); | ||
const [repairResp, setRepairResp] = useState<RepairSavedObjectResponse | null>(null); | ||
|
||
async function loadRepairList(simulate: boolean = true) { | ||
setLoading(true); | ||
try { | ||
const resp = await ml.savedObjects.repairSavedObjects(simulate); | ||
setRepairResp(resp); | ||
|
||
const count = Object.values(resp).reduce((acc, cur) => acc + Object.keys(cur).length, 0); | ||
setRepairable(count > 0); | ||
setLoading(false); | ||
return resp; | ||
} catch (error) { | ||
// this shouldn't be hit as errors are returned per-repair task | ||
// as part of the response | ||
alvarezmelissa87 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
displayErrorToast(error); | ||
setLoading(false); | ||
} | ||
return null; | ||
} | ||
|
||
useEffect(() => { | ||
loadRepairList(); | ||
}, []); | ||
|
||
async function repair() { | ||
if (repairable) { | ||
// perform the repair | ||
const resp = await loadRepairList(false); | ||
// check simulate the repair again to check that all | ||
// items have been repaired. | ||
await loadRepairList(true); | ||
|
||
if (resp === null) { | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When would the response be null? If it is null - do we need to show the user some sort of message for clarity on what's happening before returning? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it'll be |
||
} | ||
const { successCount, errorCount } = getResponseCounts(resp); | ||
if (errorCount > 0) { | ||
const title = i18n.translate('xpack.ml.management.repairSavedObjectsFlyout.repair.error', { | ||
defaultMessage: 'Some jobs cannot be repaired.', | ||
}); | ||
displayErrorToast(resp as any, title); | ||
return; | ||
} | ||
|
||
displaySuccessToast( | ||
i18n.translate('xpack.ml.management.repairSavedObjectsFlyout.repair.success', { | ||
defaultMessage: '{successCount} {successCount, plural, one {job} other {jobs}} repaired', | ||
values: { successCount }, | ||
}) | ||
); | ||
} | ||
} | ||
|
||
return ( | ||
<> | ||
<EuiFlyout maxWidth={600} onClose={onClose}> | ||
<EuiFlyoutHeader hasBorder> | ||
<EuiTitle size="m"> | ||
<h2> | ||
<FormattedMessage | ||
id="xpack.ml.management.repairSavedObjectsFlyout.headerLabel" | ||
defaultMessage="Repair saved objects" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you consider any alternative terms to 'Repair' for this functionality, such as 'Sync'? The word 'repair' could imply something is broken, which is not the case; it's just a case of needing to sync up the saved objects and jobs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This button could be renamed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My vote is to rename this to 'Sync' or 'Synchronize', and if we change the label on the button we should also change the endpoint to match in case it becomes more widely used by Kibana API users. This can all be done in a follow-up. |
||
/> | ||
</h2> | ||
</EuiTitle> | ||
</EuiFlyoutHeader> | ||
<EuiFlyoutBody> | ||
<EuiCallOut color="primary"> | ||
<EuiText size="s"> | ||
<FormattedMessage | ||
id="xpack.ml.management.repairSavedObjectsFlyout.description" | ||
defaultMessage="Repair the saved objects if they are out of sync with the machine learning jobs in Elasticsearch." | ||
/> | ||
</EuiText> | ||
</EuiCallOut> | ||
<EuiSpacer /> | ||
<RepairList repairItems={repairResp} /> | ||
</EuiFlyoutBody> | ||
<EuiFlyoutFooter> | ||
<EuiFlexGroup justifyContent="spaceBetween"> | ||
<EuiFlexItem grow={false}> | ||
<EuiButtonEmpty iconType="cross" onClick={onClose} flush="left"> | ||
<FormattedMessage | ||
id="xpack.ml.management.repairSavedObjectsFlyout.closeButton" | ||
defaultMessage="Close" | ||
/> | ||
</EuiButtonEmpty> | ||
</EuiFlexItem> | ||
<EuiFlexItem grow={false}> | ||
<EuiButton | ||
onClick={repair} | ||
fill | ||
isDisabled={repairable === false || loading === true} | ||
> | ||
<FormattedMessage | ||
id="xpack.ml.management.repairSavedObjectsFlyout.repairButton" | ||
defaultMessage="Repair" | ||
/> | ||
</EuiButton> | ||
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
</EuiFlyoutFooter> | ||
</EuiFlyout> | ||
</> | ||
); | ||
}; | ||
|
||
function getResponseCounts(resp: RepairSavedObjectResponse) { | ||
let successCount = 0; | ||
let errorCount = 0; | ||
Object.values(resp).forEach((result: SavedObjectResult) => { | ||
Object.values(result).forEach(({ success, error }) => { | ||
if (success === true) { | ||
successCount++; | ||
} else if (error !== undefined) { | ||
errorCount++; | ||
} | ||
}); | ||
}); | ||
return { successCount, errorCount }; | ||
} |
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 marked as a required bundle, but the ML plugin lists the Spaces plugin as an optional dependency.
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 fixed with 21a176c
It now checks whether the
spaces
plugin exists.