[Rest Api Compatibility] Clean up x-pack/plugin rest compat tests #74701
Conversation
|
#74843 took care of |
|
@elasticmachine update branch |
|
@elasticmachine update branch |
|
merge conflict between base and head |
pgomulka
changed the title
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
x-pack/plugin/build.gradle
Outdated
| // the test uses sparse vector - not supported | ||
| 'vectors/50_vector_stats/Usage stats on vector fields', | ||
|
|
||
| // ML - not needing compatible api |
There was a problem hiding this comment.
Can you give some more background to why ML doesn't need a compatible API? Surely users will expect all the Elasticsearch APIs to be subject to the same policies irrespective of the internal org structure of Elastic.
x-pack/plugin/build.gradle
Outdated
|
|
||
| // ML - not needing compatible api | ||
| // some are failing due to cat api being called with JSON accept header | ||
| 'ml*/*/*', |
There was a problem hiding this comment.
Can't the list of ML tests that don't work just stay as it was before instead of being expanded to include all ML tests? If not then it must be that this PR has introduced new problems for ML.
Even though the list of ML tests before was quite big, it was only a small fraction of all ML tests. Adding a total wildcard will mean others that should work are accidentally broken between now and 8.0 release.
There was a problem hiding this comment.
good suggestion. I will remove the wildcard. I mistakenly assumed that all ML endpoints are not available to end users - which obviously not true.
I will remove the wildcard and see which exactly fail now
x-pack/plugin/build.gradle
Outdated
| // ML - not needing compatible api | ||
| // some are failing due to cat api being called with JSON accept header | ||
| 'ml*/*/*', | ||
| '_apis/Test cat trained models', |
There was a problem hiding this comment.
Is this a truncation of 'ml/trained_model_cat_apis/Test cat trained models'?
There was a problem hiding this comment.
yes, good spot. it was a mistake
| @@ -39,6 +40,10 @@ public InjectHeaders(Map<String, String> headers) { | |||
| @Override | |||
| public void transformTest(ObjectNode doNodeParent) { | |||
| ObjectNode doNodeValue = (ObjectNode) doNodeParent.get(getKeyToFind()); | |||
| if(isCatOperation(doNodeValue)){ | |||
There was a problem hiding this comment.
Rather than making this class aware of "cat." names, can you introduce conditional headers to inject ?
So something like :
public InjectHeaders(Map<String, String> headers, Map<Function<ObjectNode, Boolean>, Map<String, String>> conditionalHeaders)
Then if the function key evaluates to true, then insert the associated headers.
There was a problem hiding this comment.
@jakelandis good idea, but I think this should be just set of functions and when they all evaluate to true, then we would insert the headers.
implemented this here 7ad7093 - let me know what you think
There was a problem hiding this comment.
I created a separate PR for this change
#75001
There was a problem hiding this comment.
✔️ over at #75001 , thanks for the iterations.
There was a problem hiding this comment.
LGTM (alot of the changes showing up in the diff here will be merged as #75001)
…search into compat/xpack_enable_all
|
@elasticmachine update branch |
|
@elasticmachine update branch |
this PR removes tests which are not meant to be fixed (ml/, vectors/) to a separate "not to be fixed list" so that we can see which compatible changes are meant to be implemented.
relates #51816