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

[7.x] [ILM] Add index templates to "add policy" modal in policies table #77481

Closed
wants to merge 1 commit into from

Conversation

yuliacech
Copy link
Contributor

@yuliacech yuliacech commented Sep 15, 2020

Summary

Backports #77077 to 7.9

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@cjcenizal cjcenizal changed the title [ILM] backport of #77077 [7.x] [ILM] Add index templates to "add policy" modal in policies table Sep 15, 2020
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

@yuliacech I started reviewing this and got halfway through, when I started to become uncomfortable with the level of changes that we're backporting for a patch release. This isn't a critical fix, and yet the amount of code being backported requires a high investment of effort to ensure we're not introducing any bugs. I don't think this level of investment is appropriate for the value provided by this backport. I think we should make things easy for ourselves, close this PR, and use the release notes to communicate to users that this fix ships with 7.10.

@@ -29,6 +29,7 @@ export const setupEnvironment = () => {
);

mockHttpClient.interceptors.response.use(({ data }) => data);
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary? It seems odd that the other changes in this PR would manifest a TS error here.

};

const renderAliasFormElement = () => {
console.log(policy);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a console.log snuck in here.


const renderAliasFormElement = () => {
console.log(policy);
const showAliasTextInput = policy?.policy?.phases.hot?.actions.rollover;
Copy link
Contributor

Choose a reason for hiding this comment

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

The original code defines this without the first few optional chains. Do we need them or can we match the original?

const showAliasTextInput = policy.policy.phases.hot?.actions.rollover;

@yuliacech
Copy link
Contributor Author

Thanks for your efforts with this code review, @cjcenizal ! I agree with you that the changes would be pretty extensive and a risk of regression bugs is high, especially since we don't have any converted TS code here. I also want to explore further UX improvements in this modal in this issue #77879
Closing this PR without merge.

@yuliacech yuliacech closed this Sep 22, 2020
@yuliacech yuliacech deleted the ilm_templates_backport_7.9 branch October 7, 2020 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants