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 support for ElasticSearch 7.8+ Index templates #120

Merged
merged 13 commits into from
Dec 15, 2020

Conversation

Wiston999
Copy link
Contributor

This PR adds support for /_index_template elasticsearch endpoint as requested at #88 .

The relevant changes are:

  • Add a new resource named elasticsearch_template_index keeping the original resource (elasticsearch_index_template).
    • TBH, I'm not very confident about the new resource name, as it might lead to confussion. Another alternatives would be:
      • Use elasticsearch_index_template_new: This would imply renaming from elasticsearch_index_template_new elasticsearch_index_template at some point in the provider lifecycle.
      • Use elasticsearch_index_template and rename current one to elasticsearch_index_template_legacy. This would imply a major release and breaking changes for all current users of the provider.
      • Add an extra field to elasticsearch_index_template (i.e.: legacy_format) and manage the new endpoint logic in current resource.
  • Added a new diff supress function to manage the settings field in the JSON body like in the former version (There is a new depth level in JSON body for new endpoint)
  • Added a elastic7GetVersion function that gets the ES cluster "live" version.
  • Bumped ES 7 version to 7.9.3 to be able to test the new features
  • Bumped github.com/olivere/elastic/v7 to latest version so Index*IndexTemplate services are available.

Not implemented in this PR:

  • _component_template endpoint, as the underlying library still doesn't support them.

@phillbaker
Copy link
Owner

This is great! Thanks for the PR. As an FYI, I'm currently migrating CI from travis to github actions, it might be a few days before that's finished.

I'm not very confident about the new resource name, as it might lead to confusion

Agreed, I'm not sure there's a perfect solution here. I think I'd vote for the new resource as you have it here, but elasticsearch_template_index vs. elasticsearch_index_template is too hard to differentiate. I'd vote for the new resource to be named elasticsearch_index_template_v2 or elasticsearch_composable_template, similar to the upstream issue: elastic/elasticsearch#53101.

haven't found any easy way to share the ES version from provider setup function

This might get easier with #119

@Wiston999
Copy link
Contributor Author

Hi, thanks for your quick response.

I'd vote for the new resource to be named elasticsearch_index_template_v2 or elasticsearch_composable_template, similar to the upstream issue:

I like the composable way more than the v2. What do you think about naming the new resource elasticsearch_composable_index_template (and elasticsearch_composable_component_template once it's implemented in underlying library)? I'll wait for your feedback and confirmation before starting to rename everything.

This might get easier with #119

Yes, this looks very good. If this is going to be merged soon I can wait and adapt my PR to these changes so there is no need for a new function to check elasticsearch version.

@phillbaker
Copy link
Owner

What do you think about naming the new resource elasticsearch_composable_index_template (and elasticsearch_composable_component_template once it's implemented in underlying library)

They're a touch more verbose, but on par with some resources already here. Good with me!

@Wiston999
Copy link
Contributor Author

Configured Travis on my fork, you can check test results at https://travis-ci.com/github/Wiston999/terraform-provider-elasticsearch

Copy link
Owner

@phillbaker phillbaker left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for enabling travis on your fork, good workaround for the moment. Just had a couple of questions and a nit.

docs/resources/composable_index_template.md Outdated Show resolved Hide resolved
es/resource_elasticsearch_composable_index_template.go Outdated Show resolved Hide resolved
es/util.go Outdated
@@ -136,6 +155,11 @@ func normalizedIndexLifecyclePolicy(policy map[string]interface{}) map[string]in
f := flattenMap(policy)
for k, v := range f {
f[k] = fmt.Sprintf("%v", v)
// Supress phases.delete.actions.delete.delete_searchable_snapshot = true that is included by default
// starting in 7.8
if k == "phases.delete.actions.delete.delete_searchable_snapshot" && fmt.Sprintf("%v", v) == "true" {
Copy link
Owner

Choose a reason for hiding this comment

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

Hm, will this potentially also ignore it if it's set to true by the user? I think the better approach for these is to ignore these in the diff if they come back from ES but are not in terraform state.

Copy link
Owner

Choose a reason for hiding this comment

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

added a commit to back this out for now, in order to merge the rest of the PR!

@phillbaker
Copy link
Owner

Whoops, one last ask, can you add a line to the changelog as well?

@phillbaker phillbaker merged commit 8b5a81f into phillbaker:master Dec 15, 2020
@Wiston999
Copy link
Contributor Author

Hi, thanks for merging the PR. Sorry about not being able to address your comments, I've been busy in the latest days and away from my laptop.

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.

2 participants