7.x only REST specification fixes #56736
Conversation
| @@ -36,7 +36,6 @@ | |||
| "type":"enum", | |||
| "description":"The multiplier in which to display values", | |||
| "options":[ | |||
| "", | |||
There was a problem hiding this comment.
I don't think this should be removed in 7.x. In removing this, it may introduce a breaking change to the client's API and because the major versioning of the clients is in lock step with Elasticsearch, it would be a breaking change to the client's API within a major version.
To move forward, we could special case handle an empty string enum option on size for cat.threadpool in the JSON schema.
There was a problem hiding this comment.
If it were removed, each client where it would introduce a breaking change would need to specially handle this in their generator
There was a problem hiding this comment.
Hmm... i don't see a good way to special case this in the JSON schema. However, if there is a good way I am still a bit hesitant to special case this in the schema. Splippery slope argument about too many special cases and a schema stops being a schema due to special cases ... and a real world concern of readability and keeping 7.x closer inline with master.
I think that since there exists an API with a "" and there is support for it, that "" should validate with the schema (but only for 7.x). I added schema support across the board for "" in af2780a. This will not be forward ported to master, and since all changes generally go through master first it is pretty unlikely that someone will accidentally try to support "" since on master it will fail validation. (note #55736 removed this entirely from master)
I understand the desire to not break the clients, so I think we can either 1) support "" for 7.x, or 2) hard code to ignore this file in the task itself (like we do with _common), or 3) figure out how to special case this in the schema (I will need some assistance with that).
My preference is 1 (as in the latest update to this PR) (then 2, then 3)
thoughts?
There was a problem hiding this comment.
since all changes generally go through master first it is pretty unlikely that someone will accidentally try to support "" since on master it will fail validation.
Good point; this sounds like a good argument for not needing to special case "" only to cat.threadpool in 7.x since any API changes would be backported from master which would disallow.
| @@ -44,7 +44,7 @@ | |||
| "p" | |||
| ], | |||
| "deprecated":{ | |||
There was a problem hiding this comment.
I hadn't originally back-ported the deprecation to to 7.7, but am on https://github.com/elastic/elasticsearch/pull/56737/files this will help reduce the conflict.
|
@russcam - mind taking another look w.r.t comment: #56736 (comment) |
Fixes for the REST specification specific to 7.x * remove ignore "cat.thread_pool.json" and add the "" as valid option. elastic#55984 deprecated this field since it these params here have no effect on this specific API * remove ignore "indices.put_mapping.json" by adding the required / in the path to pass validation.
Fixes for the REST specification specific to 7.x * remove ignore "cat.thread_pool.json" and add the "" as valid option. elastic#55984 deprecated this field since it these params here have no effect on this specific API * remove ignore "indices.put_mapping.json" by adding the required / in the path to pass validation.
|
Pinging @elastic/es-core-features (:Core/Features/Features) |
elasticmachine
added
the
Team:Data Management
Fixes for the REST specification specific to 7.x
ignore "cat.thread_pool.json"and remove the""as an option. deprecrate size from cat.thread_pool in json spec #55984 deprecated this field since it these params here have no effect on this specific APIignore "indices.put_mapping.json"by adding the required/in the path to pass validation.Once these are fixed #56647 should be able to be cleanly backported to 7.x