-
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
[SR] SLM create and edit policies #43390
Conversation
… for repository select field. add inline option to SectionLoading. add actions prop to SectionError
Pinging @elastic/es-ui |
This comment has been minimized.
This comment has been minimized.
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 have one concrete suggestion for improving the UX regarding the steps navigation. My other comments are copy suggestions, nit picks, and questions for clarification. I've reviewed 2/3 of the code and will review the rest tomorrow. Overall this is looking superb. The UX and functionality is exemplary. Great work!!
@@ -27,12 +40,12 @@ export const CronDaily = ({ | |||
<EuiFormRow | |||
label={( | |||
<FormattedMessage | |||
id="xpack.rollupJobs.cronEditor.cronDaily.fieldTimeLabel" | |||
id="es_ui.cronEditor.cronDaily.fieldTimeLabel" |
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.
At the risk of bikeshedding, could we use esUi
instead? Otherwise I think the naming rules become a bit confusing because I wouldn't be sure when to use underscores vs. camel case.
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 named this es_ui
because of the way kibana_react
folder was named and assumed that was the convention for folders with shared components. but now I see that a different kibana-react
was added recently, with dash instead of underscore 😆
I'll change ours to esUi
.
</EuiButtonEmpty> | ||
</EuiFlexItem> | ||
) : null} | ||
{currentStep < 3 ? ( |
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: can we use variables here instead of hardcoding the number of steps?
const lastStep = Object.keys(stepMap).length;
/* ... */
{currentStep < lastStep ? (
/* ... */
{currentStep === lastStep ? (
} = useLoadRepositories(); | ||
|
||
// State for touched inputs | ||
const [touched, setTouched] = useState({ |
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.
@sebelga Just wanted to draw your attention to this mechanism since it's similar to the work you've been doing with the form hooks lib. It's used for showing validation after a user has interacted with a field.
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.
Yes, this is yet another way of validating/displaying validation/clearing validation. So here, if I click the name input, then decide to click outside (blur) I have the validation displaying below the field. Not saying this is bad, just saying that this is new to this form and at some point, we should seek to align the validation behavior across apps (thus the form lib work 😊 ).
Like I told you, IMO (my preference) the best UX is to debounce user keystroke events and after a short delay of "silence" (and having inserted a value) display an error if necessary.
const [isAdvancedCronVisible, setIsAdvancedCronVisible] = useState<boolean>( | ||
Boolean(policy.schedule && policy.schedule !== DEFAULT_POLICY_SCHEDULE) | ||
); | ||
const [fieldToPreferredValueMap, setFieldToPreferredValueMap] = useState<any>({}); |
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.
Even though I wrote the cron editor originally, I had to re-read the code to understand what on earth the fieldToPreferredValueMap
was for. Since we're extracting this common component in this PR, I think we have a good opportunity to make this name more descriptive. What do you think? How about we change it to fieldToDefaultValueMap
? Or simply defaultValues
?
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 must say I don't understand either what "fieldToPreferredValueMap" refers to. If the <CronEditor />
is controlled it should accept a value
(object) prop. And if it's not controlled a defaultValue
(to set its initial state). Having to provide "cronExpression", "frequency", "fieldToPreferredValueMap" makes it hard to follow.
I assume that it is controlled, so I would have it all grouped under a cronState
under the <PolicyStepLogistics />
const [cronState, setCronState] = useState<CronState>({
expression: DEFAULT_POLICY_SCHEDULE,
frequency: DEFAULT_POLICY_FREQUENCY,
fieldToPreferredValueMap: {}, // still don't know what this refers to :))
});
}; | ||
|
||
const [isShowingFullIndicesList, setIsShowingFullIndicesList] = useState<boolean>(false); | ||
const displayIndices = indices |
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.
It might just be me but I had to read this a few times to parse the nested ternaries. What do you think of reducing it to a single ternary like this?
const displayIndices = (indices && typeof indices === 'string')
? indices.split(',')
: indices || undefined;
Why are we checking type? Can it be something other than a string or undefined? I wonder if indices || undefined
is necessary.
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 also had to spend some time between the isShowingFullIndicesList
, displayIndices
(which I thought first was a boolean) and ternary in the JSX
(isShowingFullIndicesList
? displayIndices
: [...displayIndices].splice(0, 10)
)
to understand how the logic was working.
What do you think of this other way of writing it?
const INDICES_PREVIEW_COUNT = 10;
const [totalIndexToDisplay, setTotalIndexToDisplay] = useState<number | undefined>(INDICES_PREVIEW_COUNT);
const indicesToArray = typeof indices === 'string'
? indices.split(',')
: indices || [];
const indicesFiltered = indicesToArray.slice(0, totalIndexToDisplay);
const hiddenIndicesCount = indicesToArray.length - INDICES_PREVIEW_COUNT;
const areAllIndicesVisible = indicesFiltered.length > INDICES_PREVIEW_COUNT;
// ... and then in the JSX
<EuiDescriptionListDescription>
{indicesToArray.length ? (
<Fragment>
<EuiText>
<ul>
{indicesFiltered.map(index => (
<li key={index}>
<EuiTitle size="xs">
<span>{index}</span>
</EuiTitle>
</li>
))}
</ul>
</EuiText>
{hiddenIndicesCount > 0 ? (
<Fragment>
<EuiSpacer size="s" />
{areAllIndicesVisible ? (
<EuiLink onClick={() => setTotalIndexToDisplay(INDICES_PREVIEW_COUNT)}>
<FormattedMessage
id="xpack.snapshotRestore.policyForm.stepReview.summaryTab.indicesCollapseAllLink"
defaultMessage="Hide {count, plural, one {# index} other {# indices}}"
values={{ count: hiddenIndicesCount }}
/>{' '}
<EuiIcon type="arrowUp" />
</EuiLink>
) : (
<EuiLink onClick={() => setTotalIndexToDisplay(undefined)}>
<FormattedMessage
id="xpack.snapshotRestore.policyForm.stepReview.summaryTab.indicesShowAllLink"
defaultMessage="Show {count} more {count, plural, one {index} other {indices}}"
values={{ count: hiddenIndicesCount }}
/>{' '}
<EuiIcon type="arrowDown" />
</EuiLink>
)}
</Fragment>
): null}
</Fragment>
) : (
<FormattedMessage
id="xpack.snapshotRestore.policyForm.stepReview.summaryTab.allIndicesValue"
defaultMessage="All indices"
/>
)}
</EuiDescriptionListDescription>
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 added this suggestion to #44036 as the same logic is used in 3 other places.
description={ | ||
<FormattedMessage | ||
id="xpack.snapshotRestore.policyForm.stepSettings.partialDescription" | ||
defaultMessage="Allows snapshots of indices with primary shards that are unavailable. Otherwise, the entire snapshot will fail." |
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.
@gchaps Same question here, should this be "Otherwise, the entire snapshot will fail if any primary shards are unavailable"?
executePolicyPrompt(policyName, () => | ||
// Wait a little bit for policy to execute before | ||
// reloading policy table and policy details | ||
setTimeout(() => { |
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.
What's the UX intention behind this timeout? Do we want to have similar behavior when hitting Run
in the table row?
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 timeout is to ensure that if the user is in History tab or clicks History tab after running, they get information about the run they just executed. there is no such information in the table, so having timeout from table action isn't necessary. I'll clarify this code comment
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.
there is no such information in the table, so having timeout from table action isn't necessary
I was thinking more if the History tab is visible and the user hits "Run" using the button in the table row.
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.
oh I see. I think this is a good example of a non-redux limitation. in a redux implementation, "Run" would be an action that would trigger data refreshes across multiple components, regardless of where the action executed from. in this implementation, the list component loads data into the table component, but the detail panel loads data independently of either of them. I'm not sure what the best approach is in this case, in order to trigger a refresh of the detail panel from an action taken inside the table. 🤔any ideas?
for this case, the Run from table is hard to access at the same time as the History tab (basically needs an ultrawide window), and there is a refresh button available above the tab as well. however I see this potentially affecting our other apps too, where there are table actions that could affect detail panel data, so I think this merits further discussion.
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.
Seems like a perfect use case of React Context
@jen-huang
@@ -80,7 +86,7 @@ export const TabSummary: React.FunctionComponent<Props> = ({ policy }) => { | |||
const fullIndicesList = | |||
indices && indices.length && indices.length > 10 ? ( | |||
<ul> | |||
{indices.map((index: string) => ( | |||
{(Array.isArray(indices) ? indices : indices.split(',')).map((index: string) => ( |
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.
We normalize indices
to an array here and on line 56. Would it be worth extracting this into a common variable?
const indicesArray = Array.isArray(indices) ? indices : indices.split(',');
@@ -207,7 +208,7 @@ export const RestoreList: React.FunctionComponent = () => { | |||
} | |||
|
|||
return ( | |||
<WithPrivileges privileges="index.*"> | |||
<WithPrivileges privileges={APP_RESTORE_INDEX_PRIVILEGES.map(name => `index.${name}`)}> |
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.
Why are we prepending index.
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.
Because privileges are namespaced by "section". This allows requiring:
- all privileges of a specific section:
"index.*"
- a specific privilege
"index.some_specific_index_privilege"
It also prevents privilege naming collision. (ES does not seem to be consistent in its naming: "cluster:admin/snapshot"
for a cluster privilege, "monitor"
for an index privilege)
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.
What is a "section"? Is it just an arbitrary identifier or are we creating formal distinctions between index privileges and cluster privileges?
If it's the latter, then I suggest changing this interface in the future (not necessarily in this PR) to make this clear and so can we avoid special strings which IMO don't communicate their meaning clearly.
<WithPrivileges indexPrivileges={APP_RESTORE_INDEX_PRIVILEGES} clusterPrivileges={EXAMPLE_REQUIRED_CLUSTER_PRIVILEGES}>
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.
A "section" is a "namespace". It allows us to group privileges. Your suggestion would not re-usable as there is an infinite number of groups. This grouping maps to our API contract, when we return the missing privileges (https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/snapshot_restore/server/routes/api/app.ts#L50).
So basically, it simply adding a prefix to the ES privileges to point to the right privileges.
</EuiLink> | ||
</p> | ||
</Fragment> | ||
<WithPrivileges privileges={APP_SLM_CLUSTER_PRIVILEGES.map(name => `cluster.${name}`)}> |
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.
Similar question 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.
Amazing job @jen-huang !! Overall it works great, I have found though a few things and made the comments in the code. Most of them are not blocker.
Here are a few other things I found:
-
I found a bug when giving a name with a
/
. (example "/hello/"). There is no way to either view or edit the template. -
In snapshot settings, when I switch to "Use index patterns", I immediately get the validation error even though I haven't touched anything yet.
- Another bug is that, if I deselect "All indices", then "Select All", then "Use Index Patterns" then "Select Indices". I am back with all indices selected, but I get a validation error saying I haven't selected any.
I haven't finished the review yet, I finished the form but I haven't looked yet at the detail panel.
minuteOptions, | ||
onChange, | ||
}) => ( | ||
<Fragment> |
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: Fragment not needed
* under the License. | ||
*/ | ||
|
||
import { i18n } from '@kbn/i18n'; |
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 we do this? If we can, then what is the purpose of the Core
providing the i18n
instance? It seems to me that i18n
should be injected.
import { SlmPolicy, SlmPolicyEs } from '../../common/types'; | ||
import { deserializeSnapshotConfig } from './'; | ||
import { SlmPolicy, SlmPolicyEs, SlmPolicyPayload } from '../types'; | ||
import { deserializeSnapshotConfig, serializeSnapshotConfig } from './'; |
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: When importing from the same folder (or subfolder), I find it clearer to indicate the file name instead of the barrel.
.snapshotRestore__policyForm__stepSettings { | ||
.euiFormRow--hasEmptyLabelSpace { | ||
min-height: auto; | ||
margin-top: $euiFontSizeXS + $euiSizeS + ($euiSizeXXL / 4); |
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 this the magic number? 😄
return; | ||
} | ||
setCurrentStep(step); | ||
setMaxCompletedStep(step - 1); |
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 agree, but using the callBack version. And can we simplify a bit the navigation logic?
const updateCurrentStep = (nextStep: number) => {
if (!validation.isValid) {
return;
}
setCurrentStep(nextStep);
setMaxCompletedStep(prev => Math.max(prev, nextStep - 1));
clearSaveError();
};
const onBack = () => {
updateCurrentStep(currentStep - 1);
};
const onNext = () => {
updateCurrentStep(currentStep + 1);
};
I just reviewed the Template UI create wizard and it would be nice to align behaviors:
- Allow clicking on any step (they are all enabled) and do the validation on step-change (preventing the change if the step is invalid)
- When in edit mode all steps should be enabled as the policy is valid (by doing previous point this will be done)
Alison also added a "cancel" button (bottom right-aligned) that I think is useful to exit the flow.
config.indices && config.indices.length > 0 ? ( | ||
<EuiLink | ||
onClick={() => { | ||
indicesOptions.forEach((option: Option) => { |
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 seems to work because the component is re-rendered, but I think it would be better to update the state with setIndicesOptions()
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 an EUI bug: elastic/eui#2071
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.
Cool. Can you add a comment pointing to it so if we come back later we can check if it has been fixed?
) : ( | ||
<EuiLink | ||
onClick={() => { | ||
indicesOptions.forEach((option: Option) => { |
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.
Same comment as above about updating the state.
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 an EUI bug: elastic/eui#2071
</EuiSelectable> | ||
) : ( | ||
<EuiComboBox | ||
options={indices.map(index => ({ label: index }))} |
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.
Here the user has decided to define index patterns to target the indices. It does not make much sense to offer him a way to enter an index name. What do you think?
Also, there is no validation on the pattern being inserted (the same validation we do in CCR when adding follower patterns). Not a blocker, as it would be much more easy to add it with the Form lib once merged.
@cjcenizal this is already the 3rd place we need to insert a pattern + validation, I think we can make it a priority to have a reusable solution 😊
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 think the suggestion list is useful to the user as a way to view matching items as they type, and it's possible they may want to do something with a pattern and single index, such as logstash-*,single-index
defaultMessage="Ignore unavailable indices" | ||
/> | ||
} | ||
checked={config.ignoreUnavailable} |
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.
defaultMessage="Allow partial indices" | ||
/> | ||
} | ||
checked={config.partial} |
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.
Same error here, it is undefined
by default instead of false
,
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.
Had a couple other minor suggestions but nothing to block on. Tested locally, code LGTM!
} | ||
|
||
public setTitle(page?: string): void { | ||
if (page && page !== 'home' && textService.breadcrumbs[page]) { |
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 seems like this could be simplified to:
if (page === 'home') {
this.changeDocTitle(`${textService.breadcrumbs.home}`);
} else if (textService.breadcrumbs[page]) {
this.changeDocTitle(`${textService.breadcrumbs[page]} - ${textService.breadcrumbs.home}`);
}
} | ||
|
||
// Remove fields with no errors | ||
validation.errors = Object.entries(validation.errors) |
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: Can we use fromEntries
here? And maybe extract a variable for use on ln 91, so we end up doing less work?
const validationErrors = Object.entries(validation.errors).filter(([key, value]) => value.length > 0);
validation.errors = Object.fromEntries(validationErrors);
validation.isValid = validationErrors.length === 0;
I found a few small UI issues that can be addressed in a later PR. Refresh button in detail panelCan we change this to a button in the footer? I feel like these actions belong together, and I think using text will make the meaning of the button clearer than the icon. Missing privileges at edit and create routesDo you want to use Index patterns combo box messagingI'm not sure what's causing this but when you try to input a duplicate pattern, the combo box says "You've selected all available options" instead of "{ITEM} has already been added", as the EUI example demonstrates. |
Re: Missing privileges at edit and create routes Re: Index patterns combo box messaging |
@sebelga I filed an ES issue to disallow Edit: I just tested a policy with the name |
Right, this occurs when the user enters the view via the URL. In terms of reproducing the combo box state, I'm logged in as the elastic user, so I believe I have all the standard permissions. If you enter "a", "b", and then "c" you should be able to repro. This might actually be a combo box bug, but I haven't dug deeper to verify that. |
OK, I investigated the combo box bug and it's a problem in EUI (elastic/eui#2258). |
💚 Build Succeeded |
💚 Build Succeeded |
@jen-huang we had already a discussion with the team about those characters in the naming and they don't want to add any restriction. This is the reason we had to do the encoding/decoding in many places in our apps. Alison just had to copy in Template UI what we did in CCR to allow any special char. This is her commit to support those chars: 8117bea |
@sebelga I recall the thread about resource names/IDs and not wanting to restrict characters globally across ES apps. However I think there is room for character restriction on a per-resource or per-feature basis. I will surface this to the ES engineers and see if this is in fact a valid place to restrict. |
💔 Build Failed |
💚 Build Succeeded |
Thank you for the reviews @sebelga and @cjcenizal, I've addressed the most important comments and fixed a few bugs found by Seb. I'm going to go ahead and merge this now for the sake of time. In a subsequent PR I will work on: cleaning up wizard step rules, removing imported i18n, and any work related to encoding policy names after following up with ES. |
* add buttons and links to create/edit policy * set up add policy form * start create policy form, including loading/error states and redirect for repository select field. add inline option to SectionLoading. add actions prop to SectionError * add snapshot name field * Change page title upon app navigation, improve breadcrumbs * Add on cancel to policy form, reorder fields * Add simple cron field * First pass at create/edit policy functionality * Adjust permissions for SLM tab * Adjust no snapshots prompt based on if policies exist or not * Add selectable indices to policy form * Move cron editor from rollup jobs to ES UI shared folder * Used shared cron editor for slm policy create/edit * Adjust copies; add duplicate schedule warning callout * Surface in progress information * Fix doc link for 7.x * Fix rollup tests * Copy edits from review * Add ES endpoint to request review * Remove unused imports * Fix i18n by cleaning up typo'd text * Remove unused import * Fix permissions and i18n * Revert change to Logistics copy * Fix bugs and PR feedback * Add cancel button to form and add comment for list * Adjust timeout comment * Fix bug with list of indices in detail panel when clicking through table * Add comment about EUI bug
Summary
This PR adds the ability to create and edit SLM policies from the UI. Create and edit is done by using a three step wizard form.
This PR also adds permission checking for the Policies tab of Snapshot and Restore. The permission missing UI can be tested by assigning the user a role that does not have the
manage_slm
cluster privilege. A user that hasmanage_slm
privilege will be able to access the Policies tab normally. This permission also affects the empty prompt in Snapshots tab (see screenshots below).Developer notes
The policies form re-uses the cron editor that was built for rollup jobs. That component and its helpers were moved to
src/plugins/es_ui_shared/public/components/cron_editor/
directory. A new i18n prefix namedes_ui
was registered that points to everything withinsrc/plugins/es_ui_shared/
.Screenshots
Policies and Snapshot tab permissions
.Policies tab when user does not have
manage_slm
Snapshot tab empty prompt when user does not have
manage_slm
(is not able to access Policies tab)Snapshot tab empty when when user does have
manage_slm
(is able to access Policies tab) and does not have any policies yetSnapshot tab empty when when user does have
manage_slm
(is able to access Policies tab) and has at least one policyCreate policy
.Entry: Policies tab empty prompt to create
Entry: Policies table create button
Step 1: Logistics, with and without at least one repository
Step 2: Snapshot settings
Step 3: Review
Edit policy
.Entry: Policies table
Entry: Policies detail
Edit wizard is the same as create, except the policy name field is disabled