-
Notifications
You must be signed in to change notification settings - Fork 98
Adding specification for the new data stream settings APIs #4416
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
base: main
Are you sure you want to change the base?
Conversation
specification/_doc_ids/table.csv
Outdated
indices-get-field-mapping,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-get-mapping | ||
indices-get-index,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-get | ||
indices-get-mapping,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-get-mapping | ||
indices-get-settings,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-get-settings | ||
indices-get-template,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-get-index-template | ||
indices-get-template-v1,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-get-template | ||
indices-open-close,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-open | ||
indices-put-data-stream-settings,https://www.elastic.co/guide/en/elasticsearch/reference/master/data-streams.html |
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.
This is the only URL in this whole file that links to a docs v2 page. @lcawl @georgewallace Does this need to be changed?
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 had just put in the placeholder we had used on the elasticsearch side. I can change it to something else. Looking now to see what fits best.
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've added a lot of minor suggestions, but the important one is related to the @doc_tag
. It would also be nice to have request and response examples per https://github.com/elastic/elasticsearch-specification/blob/main/docs/add-new-api.md#add-endpoint-request-and-response-examples
specification/indices/get_data_stream_settings/IndicesGetDataStreamSettingsRequest.ts
Show resolved
Hide resolved
specification/indices/get_data_stream_settings/IndicesGetDataStreamSettingsRequest.ts
Show resolved
Hide resolved
specification/indices/put_data_stream_settings/IndicesPutDataStreamSettingsRequest.ts
Show resolved
Hide resolved
specification/indices/put_data_stream_settings/IndicesPutDataStreamSettingsRequest.ts
Outdated
Show resolved
Hide resolved
specification/indices/put_data_stream_settings/IndicesPutDataStreamSettingsRequest.ts
Outdated
Show resolved
Hide resolved
specification/indices/put_data_stream_settings/IndicesPutDataStreamSettingsRequest.ts
Outdated
Show resolved
Hide resolved
specification/indices/put_data_stream_settings/IndicesPutDataStreamSettingsRequest.ts
Outdated
Show resolved
Hide resolved
specification/indices/put_data_stream_settings/IndicesPutDataStreamSettingsResponse.ts
Show resolved
Hide resolved
specification/indices/put_data_stream_settings/IndicesPutDataStreamSettingsResponse.ts
Outdated
Show resolved
Hide resolved
specification/indices/put_data_stream_settings/IndicesPutDataStreamSettingsResponse.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Lisa Cawley <[email protected]>
Following you can find the validation results for the APIs you have changed.
You can validate these APIs yourself by using the |
export class DataStreamSettings { | ||
/** The name of the data stream. */ | ||
name: string | ||
settings: IndexSettings |
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.
settings: IndexSettings
The example includes effective_settings
. If that's valid, shouldn't it appear here too?
@@ -305,13 +305,15 @@ indices-exists,https://www.elastic.co/docs/api/doc/elasticsearch/operation/opera | |||
indices-flush,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-flush | |||
indices-forcemerge,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-forcemerge | |||
indices-get-alias,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-get-alias | |||
indices-get-data-stream-settings,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-get-data-stream-settings |
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.
When I test a preview where this API is ga, the URL looks like this, since I think it's derived from the rest_spec_name
(or rather the operationID
in OpenAPI)
indices-get-data-stream-settings,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-get-data-stream-settings | |
indices-get-data-stream-settings,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-get-data-stream-settings |
indices-get-field-mapping,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-get-mapping | ||
indices-get-index,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-get | ||
indices-get-mapping,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-get-mapping | ||
indices-get-settings,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-get-settings | ||
indices-get-template,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-get-index-template | ||
indices-get-template-v1,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-get-template | ||
indices-open-close,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-open | ||
indices-put-data-stream-settings,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-put-data-stream-settings |
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.
When I test this in a preview, the URL is:
indices-put-data-stream-settings,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-put-data-stream-settings | |
indices-put-data-stream-settings,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-indices-put-data-stream-settings |
} | ||
/** Settings to be applied to the data stream. | ||
* @codegen_name settings */ | ||
body: IndexSettings |
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.
body: IndexSettings
This currently seems to result in a kind of recursive situation in the OpenAPI output:
"requestBody": {
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/indices._types.IndexSettings"
},
...
"indices._types.IndexSettings": {
"type": "object",
"properties": {
"index": {
"$ref": "#/components/schemas/indices._types.IndexSettings"
},
"mode": {
"type": "string"
},
...
We don't have that level of detail in our old docs (e.g. https://www.elastic.co/guide/en/elasticsearch/reference/8.18/indices-update-settings.html or https://www.elastic.co/guide/en/elasticsearch/reference/8.18/index-modules.html#index-modules-settings), so I want to call it out to ensure it's accurate.
This adds specifications for the new data stream settings APIs.