-
Notifications
You must be signed in to change notification settings - Fork 123
Add support for index creation query params #244
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
Conversation
518b9b2 to
dae0688
Compare
dae0688 to
34f654b
Compare
tobio
left a comment
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.
Nice, couple of minor comments
| } | ||
|
|
||
| func PutIndex(ctx context.Context, apiClient *clients.ApiClient, index *models.Index) diag.Diagnostics { | ||
| var includeTypeNameUnsupportedVersion = version.Must(version.NewVersion("8.0.0")) |
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.
nit: IMO it would be nice to include if this is an upper or lower bound in the name.
| var includeTypeNameUnsupportedVersion = version.Must(version.NewVersion("8.0.0")) | |
| var includeTypeNameMinUnsupportedVersion = version.Must(version.NewVersion("8.0.0")) |
| if diags := elasticsearch.PutIndex(ctx, client, &index); diags.HasError() { | ||
| params := models.PutIndexParams{ | ||
| WaitForActiveShards: d.Get("wait_for_active_shards").(string), | ||
| IncludeTypeName: d.Get("include_type_name").(bool), |
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.
I think we should throw an error if include_type_name = true when running against an 8.x server. IMO it avoids potential confusion for users.
Personally, I would probably do all the version checking in here, switch the IncludeTypeName struct property to a pointer and then set it in the create request if it's not nil in the params but I don't feel strongly about it.
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.
Fixed to return error when the version is unsupported!
Personally, I would probably do all the version checking in here, switch the IncludeTypeName struct property to a pointer and then set it in the create request if it's not nil in the params but I don't feel strongly about it.
Agreed, moved the logic here. We don't have a way to know if the value is zero value (false) or not set , I simply make it evaluate and set when include_type_name = true.
| }, | ||
| "include_type_name": { | ||
| Type: schema.TypeBool, | ||
| Description: "If true, a mapping type is expected in the body of mappings. Defaults to false.", |
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.
We should detail the version constraint here
| Description: "If true, a mapping type is expected in the body of mappings. Defaults to false.", | |
| Description: "If true, a mapping type is expected in the body of mappings. Defaults to false. Supported for Elasticsearch 7.x", |
tobio
left a comment
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.
Nice, thanks!
Resolve #242
This PR adds support for the following query params that we can set for index creation.
include_type_namewait_for_active_shardsmaster_timeouttimeouthttps://www.elastic.co/guide/en/elasticsearch/reference/7.17/indices-create-index.html#indices-create-api-query-params
📝
master_timeoutandtimeoutare used in the other apis, so it might be better to make it reusable.https://www.elastic.co/guide/en/elasticsearch/reference/current/ilm-put-lifecycle.html#ilm-put-lifecycle-query-params