Skip to content

Conversation

@aizerin
Copy link
Contributor

@aizerin aizerin commented Aug 21, 2024

resolves #673

@cla-checker-service
Copy link

cla-checker-service bot commented Aug 21, 2024

💚 CLA has been signed

Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

Thanks for adding this in. one small change to make this work when lifecycle isn't defined.

Comment on lines 316 to 321
lifecycle, diags := ExpandLifecycle(definedTempl["lifecycle"].(*schema.Set))
if diags.HasError() {
return templ, false, diags
}
if lifecycle != nil {
templ.Lifecycle = lifecycle
}
Copy link
Member

Choose a reason for hiding this comment

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

The tests are failing when lifecycle is not defined in the TF module. I'm pretty sure this should fix it, though it's totally untested.

Suggested change
lifecycle, diags := ExpandLifecycle(definedTempl["lifecycle"].(*schema.Set))
if diags.HasError() {
return templ, false, diags
}
if lifecycle != nil {
templ.Lifecycle = lifecycle
}
if lc, ok := definedTempl["lifecycle"]; ok {
lifecycle, diags := ExpandLifecycle(lc.(*schema.Set))
if diags.HasError() {
return templ, false, diags
}
if lifecycle != nil {
templ.Lifecycle = lifecycle
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for mistake. I didn't test newly created index. I fixed it in ExpandLifecycle function.

@tobio
Copy link
Member

tobio commented Aug 23, 2024

@aizerin are you able to add an entry to the CHANGELOG for this one as well please?

@aizerin
Copy link
Contributor Author

aizerin commented Aug 26, 2024

I have pushed one more fix. Tests should work now.

Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for taking the time to get this added in!

@tobio tobio merged commit fba1080 into elastic:main Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] ES.IndexTemplate.DataStream.Lifecycle.data_retention

2 participants