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.

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

Add modal to select index template on index creation #19381

Open
wants to merge 32 commits into
base: indexsetTemplates
Choose a base branch
from

Conversation

grotlue
Copy link
Contributor

@grotlue grotlue commented May 17, 2024

Description

/jpd Graylog2/graylog-plugin-enterprise#7363

This PR implements a modal to select an index set template on index set creation.

  • it is opened by default when creating a new index set
  • it can be closed and reopened
  • a template is not required to create an index set
  • when a template is applied the form values of the index set form are updated
  • the index set is still validated as is
  • when the cluster is not configured for the warm tier we show a warning in the modal
  • on cloud this modal is not shown
  • in the first segment of the modal we should "built_in" templates. The design is updated for just three as according to @tellistone we won't have more

Besides that the PR also changes the following which should be tested as well:

  • Replace defaultChecked with checked in <FormikInput>
    With the templates updating the form we need controlled Checkboxes, thus we need to use checked instead of defaultChecked

  • Preselecting the first repository in warm tier repository select

Motivation and Context

Part of: #19075

How Has This Been Tested?

Please make sure to test in the following environments:

  • with enterprise license and warm tier (repository) configuration
  • with enterprise license and without warm tier (repository) configuration
  • without enterprise license
  • on cloud
  • on prem

You can create built_in templates by creating a normal template on /system/indices/templates/create and simply changing built_in to true in the index_set_templates table in your mongodb

Screenshots (if appropriate):

Based on the mockup in #index-templates Slack channel and discussions with @tellistone
Screenshot 2024-05-29 at 15 07 41
Screenshot 2024-05-29 at 15 07 54
Screenshot 2024-05-29 at 15 08 08

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@grotlue grotlue mentioned this pull request May 17, 2024
8 tasks
@grotlue grotlue self-assigned this May 27, 2024
@grotlue grotlue marked this pull request as ready for review May 29, 2024 12:45
@grotlue grotlue requested review from ousmaneo and gally47 May 29, 2024 13:15
Copy link
Contributor

@ousmaneo ousmaneo left a comment

Choose a reason for hiding this comment

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

Tested and looks good.
I added some comments inline, a part from that it's good to go.


type Props = {
showSelectTemplateModal: boolean,
setShowSelectTemplateModal: (boolean) => void
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we name the function parameter here?

@@ -15,16 +15,36 @@
* <http://www.mongodb.com/licensing/server-side-public-license>.
*/

import * as React from 'react';
import React, { useMemo } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import React, { useMemo } from 'react';
import * as React from 'react';
import { useMemo } from 'react';

* along with this program. If not, see
* <http://www.mongodb.com/licensing/server-side-public-license>.
*/
import React, { useEffect } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import React, { useEffect } from 'react';
import * as React from 'react';
import { useEffect } from 'react';

@@ -102,7 +102,7 @@ const StyledTooltip = styled(Tooltip)(({ value }) => css`
position: absolute;
top: 0;

${value > 12
${value > 20
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move this value to a constant since we reuse it in many places.

pageSize: 3,
query: 'built_in:true',
sort: sort,
}, { enabled: true },
Copy link
Contributor

Choose a reason for hiding this comment

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

We can add the default value enabled = true to useTemplates, so we add the param only when we want to disable it.

@@ -74,66 +83,98 @@ const formatRefreshInterval = (intervalInMs : number) => {

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add an Edit button to the template details page.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants