-
Notifications
You must be signed in to change notification settings - Fork 134
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
Conversation
…rue from IL Policy
…diff supress function
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.
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
This might get easier with #119 |
Hi, thanks for your quick response.
I like the
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. |
They're a touch more verbose, but on par with some resources already here. Good with me! |
Configured Travis on my fork, you can check test results at https://travis-ci.com/github/Wiston999/terraform-provider-elasticsearch |
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.
Looks good, thanks for enabling travis on your fork, good workaround for the moment. Just had a couple of questions and a nit.
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" { |
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.
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.
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.
added a commit to back this out for now, in order to merge the rest of the PR!
Whoops, one last ask, can you add a line to the changelog as well? |
* origin/master: Moving client creation to resource to allow for provider interpolation (phillbaker#119)
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. |
This PR adds support for
/_index_template
elasticsearch endpoint as requested at #88 .The relevant changes are:
elasticsearch_template_index
keeping the original resource (elasticsearch_index_template
).elasticsearch_index_template_new
: This would imply renaming fromelasticsearch_index_template_new
elasticsearch_index_template
at some point in the provider lifecycle.elasticsearch_index_template
and rename current one toelasticsearch_index_template_legacy
. This would imply a major release and breaking changes for all current users of the provider.elasticsearch_index_template
(i.e.:legacy_format
) and manage the new endpoint logic in current resource.settings
field in the JSON body like in the former version (There is a new depth level in JSON body for new endpoint)elastic7GetVersion
function that gets the ES cluster "live" version.Index*IndexTemplate
services are available.Not implemented in this PR:
_component_template
endpoint, as the underlying library still doesn't support them.