Fix cluster.put_component_template API #5370
Conversation
|
Following you can find the validation changes against the target branch for the APIs.
You can validate these APIs yourself by using the |
| /** | ||
| * @availability stack since=8.18.0 stability=stable | ||
| * @availability serverless stability=stable | ||
| */ | ||
| data_stream_options?: DataStreamOptionsTemplate |
There was a problem hiding this comment.
I don't think this is the right place to add this field. I see the class IndexState is indeed used for the template field of component templates here:
But it's also used in the
GET {index} API response:
I'd say that the name
IndexState matches most with the second use case, and that maybe at the time its fields happened to match the template field of the PUT _component_template API, but they don't match anymore; the data_stream field is not valid in the template field for component templates (or any kind of templates for that matter, it's only valid in the root body of index templates, see here).
I see that 00be10a did try to add the data_stream_options field in some places, but it doesn't look 100% correct to me. Let me have a look and try to correct some things.
There was a problem hiding this comment.
I reverted your commit and pushed some other changes; I hope you don't mind :). My intention with my commit is to indicate what the spec should look like for these changes. I'll let it up to you if you want to make code style changes, extract parts, etc. I ran make contrib right before pushing, so I'm hoping the generated files should be ok like this. Let me know what you think!
| import { Metadata, Name, VersionNumber } from '@_types/common' | ||
| import { Duration } from '@_types/Time' | ||
| import { IndexState } from '@indices/_types/IndexState' | ||
| import { IndexTemplateMapping } from '@indices/put_index_template/IndicesPutIndexTemplateRequest' |
There was a problem hiding this comment.
This import isn't particularly pretty. I'll leave it up to you whether this is fine or if we should extract the IndexTemplateMapping class to a dedicated file. If we're extracting it (which I'm in favor of), we should probably also rename it.
| * The template to be applied which includes mappings, settings, or aliases configuration. | ||
| */ | ||
| template: IndexState | ||
| template: IndexTemplateMapping |
There was a problem hiding this comment.
Both component templates and composable/index templates define the template field. In the ES code, they both point to the same class and are thus parsed in the same way, so we can/should use the same class in the spec too. (just explaining why I made this change)
| * @availability serverless stability=stable | ||
| */ | ||
| data_stream_options?: DataStreamOptionsTemplate | null | ||
| data_stream_options?: DataStreamOptions |
There was a problem hiding this comment.
According to the description of DataStreamOptionsTemplate:
Data stream options template contains the same information as DataStreamOptions but allows them to be set explicitly to null.
Which means it's only relevant for PUT requests, and this IndexTemplateSummary/IndexTemplate is only used in retrieval APIs. That's why I changed it to DataStreamOptions.
| */ | ||
| lifecycle?: DataStreamLifecycle | ||
| /** | ||
| * @availability stack since=8.19.0 stability=stable |
There was a problem hiding this comment.
While the options are technically available in 8.18.0 (which is the version you originally used in your changes), the other data stream option changes (from 00be10a) all specify 8.19.0, which was probably done because the failure store feature flag (currently the only field in data stream options) was only removed in 8.19.0, so I'm going for consistency here.
I believe this was added in elastic/elasticsearch#117357, and the YAML tests added in elastic/elasticsearch#117357 helped surface the issue in the specification validation.
I'd be happy to have a better description.