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.

Sign up for GitHub

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

Jump to bottom

[ILM] Show forcemerge in hot when rollover is searchable snapshot is enabled #85292

Merged
merged 4 commits into from
Dec 9, 2020
Merged

[ILM] Show forcemerge in hot when rollover is searchable snapshot is enabled #85292

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
26c3a99
pivot to different rollover validation mechanism
jloleysens Dec 8, 2020
a0c382a
implement stakeholder feedback to show forcemerge in hot
jloleysens Dec 8, 2020
9293e1e
replace ternary with if..else statements
jloleysens Dec 9, 2020
1f02f17
make rollover validation test more comprehensive
jloleysens Dec 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions ...agement/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { useFormData, UseField, SelectField, NumericField } from '../../../../..

import { i18nTexts } from '../../../i18n_texts';

import { ROLLOVER_EMPTY_VALIDATION, useConfigurationIssues } from '../../../form';
import { ROLLOVER_EMPTY_VALIDATION } from '../../../form';

import { useEditPolicyContext } from '../../../edit_policy_context';

Expand All @@ -51,8 +51,6 @@ export const HotPhase: FunctionComponent = () => {
const isRolloverEnabled = get(formData, useRolloverPath);
const [showEmptyRolloverFieldsError, setShowEmptyRolloverFieldsError] = useState(false);

const { isUsingSearchableSnapshotInHotPhase } = useConfigurationIssues();

return (
<>
<EuiDescribedFormGroup
Expand Down Expand Up @@ -143,7 +141,7 @@ export const HotPhase: FunctionComponent = () => {
<UseField path={ROLLOVER_FORM_PATHS.maxSize}>
{(field) => {
const showErrorCallout = field.errors.some(
(e) => e.validationType === ROLLOVER_EMPTY_VALIDATION
(e) => e.code === ROLLOVER_EMPTY_VALIDATION
);
if (showErrorCallout !== showEmptyRolloverFieldsError) {
setShowEmptyRolloverFieldsError(showErrorCallout);
Expand Down Expand Up @@ -236,8 +234,8 @@ export const HotPhase: FunctionComponent = () => {
</ToggleFieldWithDescribedFormRow>
{isRolloverEnabled && (
<>
{<ForcemergeField phase="hot" />}
{license.canUseSearchableSnapshot() && <SearchableSnapshotField phase="hot" />}
{!isUsingSearchableSnapshotInHotPhase && <ForcemergeField phase="hot" />}
</>
)}
<SetPriorityInputField phase={hotProperty} />
Expand Down
6 changes: 6 additions & 0 deletions ...plugins/index_lifecycle_management/public/application/sections/edit_policy/form/schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ import { i18n } from '@kbn/i18n';

import { FormSchema, fieldValidators } from '../../../../shared_imports';
import { defaultSetPriority, defaultPhaseIndexPriority } from '../../../constants';
import { ROLLOVER_FORM_PATHS } from '../constants';

const rolloverFormPaths = Object.values(ROLLOVER_FORM_PATHS);

import { FormInternal } from '../types';

Expand Down Expand Up @@ -127,6 +130,7 @@ export const schema: FormSchema<FormInternal> = {
validator: ifExistsNumberGreaterThanZero,
},
],
fieldsToValidateOnChange: rolloverFormPaths,
},
max_docs: {
label: i18n.translate('xpack.indexLifecycleMgmt.hotPhase.maximumDocumentsLabel', {
Expand All @@ -141,6 +145,7 @@ export const schema: FormSchema<FormInternal> = {
},
],
serializer: serializers.stringToNumber,
fieldsToValidateOnChange: rolloverFormPaths,
},
max_size: {
label: i18n.translate('xpack.indexLifecycleMgmt.hotPhase.maximumIndexSizeLabel', {
Expand All @@ -154,6 +159,7 @@ export const schema: FormSchema<FormInternal> = {
validator: ifExistsNumberGreaterThanZero,
},
],
fieldsToValidateOnChange: rolloverFormPaths,
},
},
forcemerge: {
Expand Down
40 changes: 18 additions & 22 deletions ...ns/index_lifecycle_management/public/application/sections/edit_policy/form/validations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,33 +56,29 @@ export const ROLLOVER_EMPTY_VALIDATION = 'ROLLOVER_EMPTY_VALIDATION';
* This validator checks that and updates form values by setting errors states imperatively to
* indicate this error state.
*/
export const rolloverThresholdsValidator: ValidationFunc = ({ form }) => {
export const rolloverThresholdsValidator: ValidationFunc = ({ form, path }) => {
const fields = form.getFields();
if (
!(
fields[ROLLOVER_FORM_PATHS.maxAge].value ||
fields[ROLLOVER_FORM_PATHS.maxDocs].value ||
fields[ROLLOVER_FORM_PATHS.maxSize].value
fields[ROLLOVER_FORM_PATHS.maxAge]?.value ||
fields[ROLLOVER_FORM_PATHS.maxDocs]?.value ||
fields[ROLLOVER_FORM_PATHS.maxSize]?.value
)
) {
fields[ROLLOVER_FORM_PATHS.maxAge].setErrors([
{
validationType: ROLLOVER_EMPTY_VALIDATION,
message: i18nTexts.editPolicy.errors.maximumAgeRequiredMessage,
},
]);
fields[ROLLOVER_FORM_PATHS.maxDocs].setErrors([
{
validationType: ROLLOVER_EMPTY_VALIDATION,
message: i18nTexts.editPolicy.errors.maximumDocumentsRequiredMessage,
},
]);
fields[ROLLOVER_FORM_PATHS.maxSize].setErrors([
{
validationType: ROLLOVER_EMPTY_VALIDATION,
message: i18nTexts.editPolicy.errors.maximumSizeRequiredMessage,
},
]);
return path === ROLLOVER_FORM_PATHS.maxAge
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: might be a little easier to read using if...else instead of a nested ternary.

Copy link
Contributor

Choose a reason for hiding this comment

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

A switch/case could also work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjcenizal in this case the switch statement would look something like:

    switch (true) {
      case 1 === 1:
        return 'ok';
      case 2 === 2:
        return 'ok then';
      default:
        return 'not ok';
    }

Just curious to hear thoughts on that pattern where we have true as the deciding condition and predicate expressions per case. IMO it might be less easy to grok for the majority of readers because it is less common - WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My take is it does look pretty neat though!

Copy link
Contributor

Choose a reason for hiding this comment

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

@jloleysens I was thinking something more like this unless I'm missing something:

switch (path) {
  case ROLLOVER_FORM_PATHS.maxAge:
    return {
      code: ROLLOVER_EMPTY_VALIDATION,
      message: i18nTexts.editPolicy.errors.maximumAgeRequiredMessage,
    };

  case ROLLOVER_FORM_PATHS.maxDocs:
    return {
      code: ROLLOVER_EMPTY_VALIDATION,
      message: i18nTexts.editPolicy.errors.maximumDocumentsRequiredMessage,
    };

  default:
    return {
      code: ROLLOVER_EMPTY_VALIDATION,
      message: i18nTexts.editPolicy.errors.maximumSizeRequiredMessage,
    };
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah OK, I had a different idea. Yours is simpler!

? {
code: ROLLOVER_EMPTY_VALIDATION,
message: i18nTexts.editPolicy.errors.maximumAgeRequiredMessage,
}
: path === ROLLOVER_FORM_PATHS.maxDocs
? {
code: ROLLOVER_EMPTY_VALIDATION,
message: i18nTexts.editPolicy.errors.maximumDocumentsRequiredMessage,
}
: {
code: ROLLOVER_EMPTY_VALIDATION,
message: i18nTexts.editPolicy.errors.maximumSizeRequiredMessage,
};
} else {
fields[ROLLOVER_FORM_PATHS.maxAge].clearErrors(ROLLOVER_EMPTY_VALIDATION);
fields[ROLLOVER_FORM_PATHS.maxDocs].clearErrors(ROLLOVER_EMPTY_VALIDATION);
Expand Down