Skip to content

Conversation

@pquentin
Copy link
Member

@pquentin pquentin commented Feb 3, 2025

This is essentially #3681 but just for bulk for now. I added the _json_spec change and ran make contrib to ensure that this was not adding a new validation error.

This was added to Elasticsearch in elastic/elasticsearch#120725.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2025

Following you can find the validation results for the API you have changed.

API Status Request Response
bulk 🟢 445/445 463/463

You can validate this API yourself by using the make validate target.

@mosche
Copy link
Contributor

mosche commented Feb 4, 2025

why is this not automated @pquentin ? We can't expect to ever keep this up to date in so many different places

@mosche
Copy link
Contributor

mosche commented Feb 4, 2025

I don't think is is a feasible path forward

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2025

Following you can find the validation results for the APIs you have changed.

API Status Request Response
bulk 🟢 450/450 468/468
create 🟢 24/24 23/23
index 🟢 1154/1154 1156/1156
update 🟢 35/35 35/35

You can validate these APIs yourself by using the make validate target.

@pquentin
Copy link
Member Author

pquentin commented Feb 6, 2025

We discussed this offline - the long-term plan is that the specification is the source of truth, which means that, yes, for now, we need to port changes from the rest-api-spec.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2025

Following you can find the validation results for the APIs you have changed.

API Status Request Response
bulk 🟢 450/450 468/468
create 🟢 24/24 23/23
index 🟢 1154/1154 1156/1156
update 🟢 35/35 35/35

You can validate these APIs yourself by using the make validate target.

@pquentin pquentin merged commit 2e5a4ba into main Feb 6, 2025
10 checks passed
@pquentin pquentin deleted the include-source-on-error branch February 6, 2025 08:02
github-actions bot pushed a commit that referenced this pull request Feb 6, 2025
* Add include_source_on_error to bulk API

* Add create, index and update

(cherry picked from commit 2e5a4ba)
github-actions bot pushed a commit that referenced this pull request Feb 6, 2025
* Add include_source_on_error to bulk API

* Add create, index and update

(cherry picked from commit 2e5a4ba)
github-actions bot pushed a commit that referenced this pull request Feb 6, 2025
* Add include_source_on_error to bulk API

* Add create, index and update

(cherry picked from commit 2e5a4ba)
Copy link
Contributor

@mosche mosche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @pquentin, just realised my review was still pending 🤦
Actually did index, create and update locally, but honestly was rather amazed about manually maintaining even stuff like specLocation 😮

"description": "True or false if to include the document source in the error message in case of parsing errors.",
"name": "include_source_on_error",
"required": false,
"serverDefault": false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"serverDefault": false,
"serverDefault": true,

"description": "True or false if to include the document source in the error message in case of parsing errors.",
"name": "include_source_on_error",
"required": false,
"serverDefault": false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"serverDefault": false,
"serverDefault": true,

query_parameters: {
/**
* True or false if to include the document source in the error message in case of parsing errors.
* @server_default false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @server_default false
* @server_default true

}
],
"specLocation": "_global/bulk/BulkRequest.ts#L32-L242"
"specLocation": "_global/bulk/BulkRequest.ts#L32-L247"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this also updated manually? What is this used for?

@pquentin
Copy link
Member Author

pquentin commented Feb 6, 2025

I only manually updated the specification/ directory, the rest was generated by make contrib. But if you don't we have automation that will fix it, no worries!

pquentin added a commit that referenced this pull request Feb 6, 2025
* Add include_source_on_error to bulk API

* Add create, index and update

(cherry picked from commit 2e5a4ba)

Co-authored-by: Quentin Pradet <[email protected]>
pquentin added a commit that referenced this pull request Feb 6, 2025
* Add include_source_on_error to bulk API

* Add create, index and update

(cherry picked from commit 2e5a4ba)

Co-authored-by: Quentin Pradet <[email protected]>
pquentin added a commit that referenced this pull request Feb 6, 2025
* Add include_source_on_error to bulk API

* Add create, index and update

(cherry picked from commit 2e5a4ba)

Co-authored-by: Quentin Pradet <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants