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

Allow additive mapping changes to be updated without recreation #339

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

coro
Copy link

@coro coro commented Feb 27, 2023

Context

Prior to this change, modifying the mapping on an index with this provider would result in a recreation of the index every time, regardless of the diff between the old and new mapping.

In comparison, the ElasticSearch API does allow you to modify an existing mapping if the changes are additive, i.e. they involve the addition of a new field onto the existing mapping.

As of this change, if a new mapping is terraform apply-ed to an index, the provider will do the PUT against the API to update the mapping, allowing for the new field without the need for recreation. Notes that the recreation will still occur if the changes are not purely additive - if a field is left modified or removed as a result of the change, the index will still be recreated.

Additional context

The decision as to whether a change is additive or not is from the library github.com/nsf/jsondiff. If the library considers the new mapping to have NoMatch with the old, it will trigger a recreate in the CustomizeDiff hook of the SDK. If it considers the two to have a FullMatch or SuperSetMatch relationship, the Update function will do the PUT against the API.

This PR is related to #229.

@coro
Copy link
Author

coro commented Feb 28, 2023

@phillbaker FYI, here is the PR I mentioned on #229.

As a deliberate design choice, we followed the pattern of only using the diff in Terraform's state to compare mappings before and after apply (as opposed to looking up the true state from ES). This is similar to other providers (e.g. AWS's provider)

This means if you modify the mapping outside of Terraform, this provider continues to not be aware of those changes, resulting in a silent failure if there is conflict (same experience as right now). Ideally we would like to warn the user if there is a difference between terraform's view of the current state vs the true current state of ES, but we there is implementation complexities involved.

Any recommendations/opinions you have would be appreciated. To some extent it's a philosophical question as to whether a provider should look up true state or just rely on Terraform's.

@coro
Copy link
Author

coro commented Mar 6, 2023

@phillbaker Looks like the version of golangci-lint in the GitHub action flow is quite old - 1.29.0. I ran with the latest version and my code passes just fine. Can I bump to the latest linter version as part of this PR, or would you prefer something else?

@phillbaker
Copy link
Owner

Thanks for the update, I'll take a look to see if I can bump it without issues separately.

@phillbaker
Copy link
Owner

Hi @coro I've bumped the lint version on master, please update when you have a chance.

@coro
Copy link
Author

coro commented Mar 13, 2023

Thanks @phillbaker ! I've rebased onto your change now.

@coro
Copy link
Author

coro commented Mar 22, 2023

@phillbaker I'm not sure what's happening with the linter that keeps failing - I've been running it locally with no issues and can't reproduce the failures seen here. I've removed the dot imports which is the latest thing it was complaining about and hopefully it will pass this time, but I'm uncertain how to recreate exactly the config it runs with in CI if not.

@phillbaker
Copy link
Owner

I had some issues running the linter locally, which I've now resolved. Taking a look at CI.

@evilr00t
Copy link

Hi @phillbaker, any update on this one. Is it possible to merge it?

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.

None yet

3 participants