Add delete data stream action and detail panel #68919
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cjcenizal
force-pushed
the
data-streams/delete-action
branch
2 times, most recently
from
95ee364
to
f77081e
Compare
cjcenizal
changed the title
cjcenizal
added
Feature:Index Management
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
cjcenizal
force-pushed
the
data-streams/delete-action
branch
3 times, most recently
from
1ac88d8
to
95989df
Compare
botelastic
bot
added
the
Team:Fleet
|
Pinging @elastic/ingest-management (Team:Ingest Management) |
…templates tab and update tests.
… index templates or Ingest Manager dependent upon its presence.
cjcenizal
force-pushed
the
data-streams/delete-action
branch
from
95989df
to
4670f98
Compare
cjcenizal
commented
Jun 23, 2020
| /> | ||
| </EuiDescriptionListTitle> | ||
|
|
||
| <EuiDescriptionListDescription>{timeStampField.name}</EuiDescriptionListDescription> |
There was a problem hiding this comment.
Note that this will render blank until the ES snapshot is updated.
cjcenizal
commented
Jun 23, 2020
| export type IngestManagerSetup = void; | ||
| // We need to provide an object instead of void so that dependent plugins know when Ingest Manager | ||
| // is disabled. | ||
| export interface IngestManagerSetup { |
There was a problem hiding this comment.
@elastic/ingest-management I made this change so we could inspect whether Ingest Manager was disabled or not. When it's disabled, the Ingest Manager setup dependency will be undefined. I'm happy to change this return value to anything you'd prefer.
|
@elasticmachine merge upstream |
jloleysens
approved these changes
Jun 24, 2020
There was a problem hiding this comment.
Great work @cjcenizal !
Left some comments about what I think can be improved in the code. Tested locally, specifically with the ingest manager not being enabled workflow.
Thanks for adding test coverage!
| const { actions, exists } = testBed; | ||
| test('when Ingest Manager is disabled, goes to index templates tab when "Get started" link is clicked', async () => { | ||
| await act(async () => { | ||
| testBed = await setup({ |
There was a problem hiding this comment.
Nit; this should be able to sit outside of act.
| @@ -74,7 +99,7 @@ export const DataStreamDetailPanel: React.FunctionComponent<Props> = ({ | |||
| > | |||
| <EuiFlyoutHeader> | |||
| <EuiTitle size="m"> | |||
| <h2 id="dataStreamDetailPanelTitle" data-test-subj="title"> | |||
| <h2 id="dataStreamDetailPanelTitle" data-test-subj="dataStreamDetailPanelTitle"> | |||
There was a problem hiding this comment.
Nit; I think if we wanted to stick with title we could use the find test utility's . notation. So the flyout could be dataStreamFlyout and this would be title. Then in the test we could have find('dataStreamFlyout.title')
There was a problem hiding this comment.
Good point! I prefer to use a selector that references a specific element instead of depending upon context, so I'm going to leave this as-is for now.
| }} | ||
| /> | ||
| defaultMessage="Data streams represent collections of time series indices." | ||
| />{' '} |
There was a problem hiding this comment.
Nit; not sure whether this space is intentional?
There was a problem hiding this comment.
We need this space to separate the two sentences. I'll add a comment.
Comment on lines
15
to
21
| export const DeleteDataStreamConfirmationModal = ({ | ||
| dataStreams, | ||
| callback, | ||
| }: { | ||
| dataStreams: string[]; | ||
| callback: (data?: { hasDeletedDataStreams: boolean }) => void; | ||
| }) => { |
There was a problem hiding this comment.
Nit; could we make use of React's FunctionComponent type util here? And create a separate Props interface. So the end result will be something like:
interface Props {
dataStreams: string[];
callback: (data?: { hasDeletedDataStreams: boolean }) => void;
}
export const DeleteDataStreamConfirmationModal: FunctionComponent<Props> = ({
dataStreams,
callback,
}) => { ...This also gives type security in the return value of the component (i.e., it can only return valid component-like things) and IMO tidies things up a bit.
One comment would be that we change callback to onConfirm. I would also move the deletion logic up to the parent component and then we can avoid hasDeletedDataStreams which seems like a prop that is compressing quite a lot of information to consumers.
There was a problem hiding this comment.
Good points! I'll make these changes. Thanks.
There was a problem hiding this comment.
I've incorporated your other suggestions, but I've decided to leave the deletion logic encapsulated inside of the delete modal. This logic is will be the same regardless of the consumer (which can be the table or the detail panel), so I think the added complexity of hasDeletedDataStreams is worth the hidden complexity of implementing the deletion logic in multiple places. FWIW this is a pattern we've used for most (maybe all) of our confirmation modals so if we change this pattern I think we should consider it in all of our apps.
| return useRequest<DataStream[]>({ | ||
| path: `${API_BASE_PATH}/data_stream/${encodeURIComponent(name)}`, | ||
| return useRequest<DataStream>({ | ||
| path: `${API_BASE_PATH}/data_streams/${encodeURIComponent(name)}`, |
There was a problem hiding this comment.
I see the type annotation was changed to not being an array but the endpoint is pluralised, is this intended to be the case?
There was a problem hiding this comment.
Yes, this was a deliberate change to align with our other Index Management APIs, which consistently use plural nouns regardless of whether you're fetching all or one of a given entity.
|
I can't comment on the code, but the screenshot looks good :) |
jfsiii
approved these changes
Jun 24, 2020
| // We need to provide an object instead of void so that dependent plugins know when Ingest Manager | ||
| // is disabled. | ||
| // eslint-disable-next-line @typescript-eslint/no-empty-interface | ||
| export interface IngestManagerSetup {} |
There was a problem hiding this comment.
I didn't realize the current code would prevent consumers from even testing if it was enabled
cjcenizal
force-pushed
the
data-streams/delete-action
branch
2 times, most recently
from
39f8aeb
to
b1d34e6
Compare
- Add delete action to flyout. - Always render the detail panel when deep-linked, even if table is loading, errored, or has no data streams. - Fix tests and TS errors,
cjcenizal
force-pushed
the
data-streams/delete-action
branch
from
b1d34e6
to
12a421f
Compare
cjcenizal
added a commit
that referenced
this pull request
Jun 25, 2020
Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
This was referenced Jun 25, 2020
cjcenizal
added
release_note:skip
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / kibana-xpack-agent / Firefox XPack UI Functional Tests.x-pack/test/functional/apps/infra/home_page·ts.InfraOps app Home page with metrics present renders the waffle map for dates with dataStandard OutStack TraceKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/index_management/home_page·ts.Index Management app Home page Component templates renders the component templates tabStandard OutStack TraceKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/security/doc_level_security_roles·js.security app dls user East should only see EAST docStandard OutStack Traceand 5 more failures, only showing the first 3. Build metrics
History
To update your PR or re-run it, just comment with: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Builds upon #67806
Changes
Aside from what's mentioned in the title, I also changed the wording and CTA in the empty prompt based on stakeholder feedback.
However, if Ingest Manager is disabled, the prompt guides the user to index templates.
Note that the detail panel always opens if deep-linked, regardless of whether the table is loading, has an error, or contains no data streams. I believe this is a good UX, because the user expects a deep-link to show a detail panel regardless of these contextual states.
Testing
To test, create some data streams in Console and see them in Index Management.
Release note (moved to original: #67806)
We added the ability to delete a data stream from the UI, and a panel for digging into a data stream's advanced details.