opensearch-project / terraform-provider-opensearch

https://registry.terraform.io/providers/opensearch-project/opensearch
Apache License 2.0
73 stars 56 forks source link

feat(knn_config): support knn config in 2.x indices #118

Closed mcarroll1 closed 9 months ago

mcarroll1 commented 10 months ago

Description

It appears from the test suite that other boolean settings such as load_fixed_bitset_filters_eagerly would also see issue without this feature. Please review for golang best practices/style as well. Thank you!

Issues Resolved

Closes: https://github.com/opensearch-project/terraform-provider-opensearch/issues/75

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

mcarroll1 commented 10 months ago

Sorry, I was having some trouble with the linting but I think I got it

prudhvigodithi commented 9 months ago

Hey @macohen the unit tests fail, can you please take a look ? Error Unexpected differences in acceptance test HCL content formatting., AFAIK this error should fix when ran terrafmt fmt. Ref: https://github.com/opensearch-project/terraform-provider-opensearch/blob/main/script/test-terraform-fmt @bbarani

mcarroll1 commented 9 months ago

Hey @macohen the unit tests fail, can you please take a look ? Error Unexpected differences in acceptance test HCL content formatting., AFAIK this error should fix when ran terrafmt fmt. Ref: https://github.com/opensearch-project/terraform-provider-opensearch/blob/main/script/test-terraform-fmt @bbarani

Hi @prudhvigodithi . Is there any action required on my part for now? The terramt fmt should be resolved from the maintainers' side?

prudhvigodithi commented 9 months ago

Hi @prudhvigodithi . Is there any action required on my part for now? The terramt fmt should be resolved from the maintainers' side?

Hey @mcarroll1 the fix should be part of the PR, can you please take a look at the error?

Unexpected differences in acceptance test HCL content formatting.
To see the full differences, run: terrafmt diff ./provider --pattern '*_test.go'
Error: Process completed with exit code 1.
mcarroll1 commented 9 months ago

Hi @prudhvigodithi . Is there any action required on my part for now? The terramt fmt should be resolved from the maintainers' side?

Hey @mcarroll1 the fix should be part of the PR, can you please take a look at the error?

Unexpected differences in acceptance test HCL content formatting.
To see the full differences, run: terrafmt diff ./provider --pattern '*_test.go'
Error: Process completed with exit code 1.

Ok, I believe I have resolved the terraform formatting issue

mcarroll1 commented 9 months ago

Hi @prudhvigodithi . Is there anything else that needs to be done for this PR?

prudhvigodithi commented 9 months ago

Thanks @mcarroll1 LGTM, one qq, as shown in the example https://opensearch.org/docs/latest/search-plugins/knn/approximate-knn/#get-started-with-approximate-k-nn can we not configure the K-NN settings via the body example https://github.com/opensearch-project/terraform-provider-opensearch/blob/main/provider/resource_opensearch_index_test.go#L198-L212? If so adding settings like index_knn_algo_param_ef_search outside and "knn.algo_param.ef_search" inside the body have a race condition? Just want to ensure the user just uses the direct terraform key-value pairs.

Thank you

Adding @bbarani @rblcoder @peterzhuamazon

mcarroll1 commented 9 months ago

@prudhvigodithi From the opensearch docs:

Settings and mappings that you specify directly in the create index request override any settings or mappings specified in an index template and its component templates.

I believe this should handle this case. Is this something we can create a test for?

prudhvigodithi commented 9 months ago

Thanks @mcarroll1, I'm good, merging now.

prudhvigodithi commented 7 months ago

This feature should be now in the v2.2.0 https://registry.terraform.io/providers/opensearch-project/opensearch/2.2.0, please check, thanks @mcarroll1 for you contribution.