Closed jedrazb closed 1 week ago
Following you can find the validation results for the APIs you have changed.
API | Status | Request | Response |
---|---|---|---|
connector.check_in |
:white_circle: | Missing test | Missing test |
connector.delete |
:white_circle: | Missing test | Missing test |
connector.get |
:white_circle: | Missing test | Missing test |
connector.last_sync |
:white_circle: | Missing test | Missing test |
connector.list |
:white_circle: | Missing test | Missing test |
connector.post |
:white_circle: | Missing test | Missing test |
connector.put |
:white_circle: | Missing test | Missing test |
connector.secret_delete |
:orange_circle: | Missing type | Missing type |
connector.secret_get |
:orange_circle: | Missing type | Missing type |
connector.secret_post |
:orange_circle: | Missing type | Missing type |
connector.secret_put |
:orange_circle: | Missing type | Missing type |
connector.sync_job_cancel |
:white_circle: | Missing test | Missing test |
connector.sync_job_check_in |
:orange_circle: | Missing type | Missing type |
connector.sync_job_claim |
:orange_circle: | Missing type | Missing type |
connector.sync_job_delete |
:white_circle: | Missing test | Missing test |
connector.sync_job_error |
:orange_circle: | Missing type | Missing type |
connector.sync_job_get |
:white_circle: | Missing test | Missing test |
connector.sync_job_list |
:white_circle: | Missing test | Missing test |
connector.sync_job_post |
:white_circle: | Missing test | Missing test |
connector.sync_job_update_stats |
:orange_circle: | Missing type | Missing type |
connector.update_active_filtering |
:white_circle: | Missing test | Missing test |
connector.update_api_key_id |
:white_circle: | Missing test | Missing test |
connector.update_configuration |
:white_circle: | Missing test | Missing test |
connector.update_error |
:white_circle: | Missing test | Missing test |
connector.update_features |
:orange_circle: | Missing type | Missing type |
connector.update_filtering_validation |
:white_circle: | Missing test | Missing test |
connector.update_filtering |
:white_circle: | Missing test | Missing test |
connector.update_index_name |
:white_circle: | Missing test | Missing test |
connector.update_name |
:white_circle: | Missing test | Missing test |
connector.update_native |
:white_circle: | Missing test | Missing test |
connector.update_pipeline |
:white_circle: | Missing test | Missing test |
connector.update_scheduling |
:white_circle: | Missing test | Missing test |
connector.update_service_type |
:white_circle: | Missing test | Missing test |
connector.update_status |
:white_circle: | Missing test | Missing test |
You can validate these APIs yourself by using the make validate
target.
Hey @l-trotta π I see you assigned yourself to the issue. I tried addressing the concerns we discussed in the last call. Happy to answer any questions and address any other concerns!
EDIT: I see there are some failing tests, let me address it first
Following you can find the validation results for the APIs you have changed.
API | Status | Request | Response |
---|---|---|---|
connector.check_in |
:green_circle: | 3/3 | 3/3 |
connector.delete |
:red_circle: | 3/9 | 9/9 |
connector.get |
:red_circle: | 59/59 | 1/59 |
connector.last_sync |
:green_circle: | 7/7 | 7/7 |
connector.list |
:red_circle: | 19/19 | 3/19 |
connector.post |
:red_circle: | 7/7 | 1/7 |
connector.put |
:red_circle: | 10/12 | 1/12 |
connector.secret_delete |
:orange_circle: | Missing type | Missing type |
connector.secret_get |
:orange_circle: | Missing type | Missing type |
connector.secret_post |
:orange_circle: | Missing type | Missing type |
connector.secret_put |
:orange_circle: | Missing type | Missing type |
connector.sync_job_cancel |
:green_circle: | 3/3 | 3/3 |
connector.sync_job_check_in |
:orange_circle: | Missing type | Missing type |
connector.sync_job_claim |
:orange_circle: | Missing type | Missing type |
connector.sync_job_delete |
:green_circle: | 4/4 | 4/4 |
connector.sync_job_error |
:orange_circle: | Missing type | Missing type |
connector.sync_job_get |
:green_circle: | 22/22 | 22/22 |
connector.sync_job_list |
:red_circle: | 11/12 | 12/12 |
connector.sync_job_post |
:green_circle: | 50/50 | 50/50 |
connector.sync_job_update_stats |
:orange_circle: | Missing type | Missing type |
connector.update_active_filtering |
:green_circle: | 1/1 | 1/1 |
connector.update_api_key_id |
:green_circle: | 4/4 | 4/4 |
connector.update_configuration |
:red_circle: | 5/9 | 9/9 |
connector.update_error |
:green_circle: | 4/4 | 4/4 |
connector.update_features |
:orange_circle: | Missing type | Missing type |
connector.update_filtering_validation |
:green_circle: | 3/3 | 3/3 |
connector.update_filtering |
:red_circle: | 11/12 | 12/12 |
connector.update_index_name |
:green_circle: | 4/4 | 4/4 |
connector.update_name |
:green_circle: | 4/4 | 4/4 |
connector.update_native |
:green_circle: | 3/3 | 3/3 |
connector.update_pipeline |
:green_circle: | 3/3 | 3/3 |
connector.update_scheduling |
:green_circle: | 3/3 | 3/3 |
connector.update_service_type |
:green_circle: | 2/2 | 2/2 |
connector.update_status |
:green_circle: | 3/3 | 3/3 |
You can validate these APIs yourself by using the make validate
target.
Hey @jedrazb, this is good timing because we just managed to add Connectors YAML tests to the flight recorder, so I reran validation and you can see the results in the above comment.
I can report that compared to the current state, this pull requests:
last_sync
, post
and update_name
requestssync_job_get
responsesThe rest is unchanged. I can open an issue listing the failures to not block this pull request which definitely improves things.
Hey @pquentin ! The changes that fix remaining issues should be minimal, I'm having issues running make validate
locally due to vault permission issues. I will ask more in Slack!
Following you can find the validation results for the APIs you have changed.
API | Status | Request | Response |
---|---|---|---|
connector.check_in |
:green_circle: | 3/3 | 3/3 |
connector.delete |
:green_circle: | 9/9 | 9/9 |
connector.get |
:red_circle: | 59/59 | 1/59 |
connector.last_sync |
:green_circle: | 7/7 | 7/7 |
connector.list |
:red_circle: | 19/19 | 3/19 |
connector.post |
:green_circle: | 7/7 | 7/7 |
connector.put |
:green_circle: | 12/12 | 12/12 |
connector.secret_delete |
:orange_circle: | Missing type | Missing type |
connector.secret_get |
:orange_circle: | Missing type | Missing type |
connector.secret_post |
:orange_circle: | Missing type | Missing type |
connector.secret_put |
:orange_circle: | Missing type | Missing type |
connector.sync_job_cancel |
:green_circle: | 3/3 | 3/3 |
connector.sync_job_check_in |
:orange_circle: | Missing type | Missing type |
connector.sync_job_claim |
:orange_circle: | Missing type | Missing type |
connector.sync_job_delete |
:green_circle: | 4/4 | 4/4 |
connector.sync_job_error |
:orange_circle: | Missing type | Missing type |
connector.sync_job_get |
:green_circle: | 22/22 | 22/22 |
connector.sync_job_list |
:green_circle: | 12/12 | 12/12 |
connector.sync_job_post |
:green_circle: | 50/50 | 50/50 |
connector.sync_job_update_stats |
:orange_circle: | Missing type | Missing type |
connector.update_active_filtering |
:green_circle: | 1/1 | 1/1 |
connector.update_api_key_id |
:green_circle: | 4/4 | 4/4 |
connector.update_configuration |
:red_circle: | 6/9 | 9/9 |
connector.update_error |
:green_circle: | 4/4 | 4/4 |
connector.update_features |
:orange_circle: | Missing type | Missing type |
connector.update_filtering_validation |
:green_circle: | 3/3 | 3/3 |
connector.update_filtering |
:green_circle: | 12/12 | 12/12 |
connector.update_index_name |
:green_circle: | 4/4 | 4/4 |
connector.update_name |
:green_circle: | 4/4 | 4/4 |
connector.update_native |
:green_circle: | 3/3 | 3/3 |
connector.update_pipeline |
:green_circle: | 3/3 | 3/3 |
connector.update_scheduling |
:green_circle: | 3/3 | 3/3 |
connector.update_service_type |
:green_circle: | 2/2 | 2/2 |
connector.update_status |
:green_circle: | 3/3 | 3/3 |
You can validate these APIs yourself by using the make validate
target.
Now it looks better! Two issues:
connector.get/list
endpoint responses - we are adressing this for 8.16
in https://github.com/elastic/elasticsearch/pull/110543:
null
(to match the status quo from before). When handling the response, response definition is unhappy about null
fields which are actually optional. If this doesn't break client we can probably ignore this until we change this in ES (?). null
properties have been written to connector index with direct index access since 8.8 so there are tons of connectors with fields set to null
connector.update_configuration
null
tooltip being passed in the body (we could make it just optional in server implementation)I think we can address those issues as a followup.
does this field exist only in the connector response? https://github.com/elastic/elasticsearch/blob/e1a091f1a2a7444873a1887c686186af580649e3/x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/connector/Connector.java#L211
I'm starting to consider mapping the connector request and response in two different classes, as you can see from the validator:
connector.get
| :red_circle: | 59/59 | 1/59 |
this is because in the response many types are not optional, for example
api_key_id?: string
in the request
is actually
api_key_id: string | null
in the response
Following you can find the validation results for the APIs you have changed.
API | Status | Request | Response |
---|---|---|---|
connector.check_in |
:green_circle: | 3/3 | 3/3 |
connector.delete |
:green_circle: | 9/9 | 9/9 |
connector.get |
:red_circle: | 60/60 | 59/60 |
connector.last_sync |
:green_circle: | 7/7 | 7/7 |
connector.list |
:green_circle: | 19/19 | 19/19 |
connector.post |
:green_circle: | 7/7 | 7/7 |
connector.put |
:green_circle: | 13/13 | 13/13 |
connector.secret_delete |
:orange_circle: | Missing type | Missing type |
connector.secret_get |
:orange_circle: | Missing type | Missing type |
connector.secret_post |
:orange_circle: | Missing type | Missing type |
connector.secret_put |
:orange_circle: | Missing type | Missing type |
connector.sync_job_cancel |
:green_circle: | 3/3 | 3/3 |
connector.sync_job_check_in |
:orange_circle: | Missing type | Missing type |
connector.sync_job_claim |
:orange_circle: | Missing type | Missing type |
connector.sync_job_delete |
:green_circle: | 4/4 | 4/4 |
connector.sync_job_error |
:orange_circle: | Missing type | Missing type |
connector.sync_job_get |
:green_circle: | 22/22 | 22/22 |
connector.sync_job_list |
:green_circle: | 12/12 | 12/12 |
connector.sync_job_post |
:green_circle: | 50/50 | 50/50 |
connector.sync_job_update_stats |
:orange_circle: | Missing type | Missing type |
connector.update_active_filtering |
:green_circle: | 1/1 | 1/1 |
connector.update_api_key_id |
:green_circle: | 4/4 | 4/4 |
connector.update_configuration |
:red_circle: | 8/9 | 9/9 |
connector.update_error |
:green_circle: | 4/4 | 4/4 |
connector.update_features |
:orange_circle: | Missing type | Missing type |
connector.update_filtering_validation |
:green_circle: | 3/3 | 3/3 |
connector.update_filtering |
:green_circle: | 12/12 | 12/12 |
connector.update_index_name |
:green_circle: | 4/4 | 4/4 |
connector.update_name |
:green_circle: | 4/4 | 4/4 |
connector.update_native |
:green_circle: | 3/3 | 3/3 |
connector.update_pipeline |
:green_circle: | 3/3 | 3/3 |
connector.update_scheduling |
:green_circle: | 3/3 | 3/3 |
connector.update_service_type |
:green_circle: | 2/2 | 2/2 |
connector.update_status |
:green_circle: | 3/3 | 3/3 |
You can validate these APIs yourself by using the make validate
target.
Hey @l-trotta π As we discussed in the call, I'm updating the spec to address main issues:
I'm starting to consider mapping the connector request and response in two different classes ++ Actually
Connector
type was only used to describe responses, so I updated the type accordingly.
I had to put tooltip
property as string | null
for the automated tests to pass. We always return tooltip
field from the ES (even if it's null).
I marked last_sync_error
and last_access_control_sync_error
as just optional, until we address https://github.com/elastic/elasticsearch/pull/110543 in ES.
There are just 2 failing test cases related to crawler state (in connector doc!). Due to historical reasons, some parts of the crawler state were, and still are, stored in the connector index. However, it was agreed that the connector API should not interact with the crawler, so those tests should be removed from ES. I donβt think these failures should block us in this PR, as this is a quirky edge case. Can explain more if needed :)
Hey @jedrazb, got it we'll wait for the yaml tests to be fixed!
One thing I found: in the connector POST request there should be the connector id path parameter, to allow defining the connector custom name.
Also: sync_cursor?: Dictionary<string, UserDefinedValue>
-> the server value for this is Object, and the return value for this when there's no data is null
instead of what the server usually sends as an empty dictionary which is {}
. This is problematic for some static clients that have no way of dealing with null dictionaries, so I'd suggest changing this to sync_cursor?: UserDefinedValue
, which simplifies it.
Another thing: there are some properties in ConnectorConfigProperties that should be nullable. (for example default_value, depends_on...)
got it we'll wait for the yaml tests to be fixed!
Okok I see, let me get rid of those tests from the server first.
Following you can find the validation results for the APIs you have changed.
API | Status | Request | Response |
---|---|---|---|
connector.check_in |
:green_circle: | 3/3 | 3/3 |
connector.delete |
:green_circle: | 9/9 | 9/9 |
connector.get |
:red_circle: | 60/60 | 59/60 |
connector.last_sync |
:green_circle: | 7/7 | 7/7 |
connector.list |
:green_circle: | 19/19 | 19/19 |
connector.post |
:green_circle: | 7/7 | 7/7 |
connector.put |
:green_circle: | 13/13 | 13/13 |
connector.secret_delete |
:orange_circle: | Missing type | Missing type |
connector.secret_get |
:orange_circle: | Missing type | Missing type |
connector.secret_post |
:orange_circle: | Missing type | Missing type |
connector.secret_put |
:orange_circle: | Missing type | Missing type |
connector.sync_job_cancel |
:green_circle: | 3/3 | 3/3 |
connector.sync_job_check_in |
:orange_circle: | Missing type | Missing type |
connector.sync_job_claim |
:orange_circle: | Missing type | Missing type |
connector.sync_job_delete |
:green_circle: | 4/4 | 4/4 |
connector.sync_job_error |
:orange_circle: | Missing type | Missing type |
connector.sync_job_get |
:green_circle: | 22/22 | 22/22 |
connector.sync_job_list |
:green_circle: | 12/12 | 12/12 |
connector.sync_job_post |
:green_circle: | 50/50 | 50/50 |
connector.sync_job_update_stats |
:orange_circle: | Missing type | Missing type |
connector.update_active_filtering |
:green_circle: | 1/1 | 1/1 |
connector.update_api_key_id |
:green_circle: | 4/4 | 4/4 |
connector.update_configuration |
:red_circle: | 8/9 | 9/9 |
connector.update_error |
:green_circle: | 4/4 | 4/4 |
connector.update_features |
:orange_circle: | Missing type | Missing type |
connector.update_filtering_validation |
:green_circle: | 3/3 | 3/3 |
connector.update_filtering |
:green_circle: | 12/12 | 12/12 |
connector.update_index_name |
:green_circle: | 4/4 | 4/4 |
connector.update_name |
:green_circle: | 4/4 | 4/4 |
connector.update_native |
:green_circle: | 3/3 | 3/3 |
connector.update_pipeline |
:green_circle: | 3/3 | 3/3 |
connector.update_scheduling |
:green_circle: | 3/3 | 3/3 |
connector.update_service_type |
:green_circle: | 2/2 | 2/2 |
connector.update_status |
:green_circle: | 3/3 | 3/3 |
You can validate these APIs yourself by using the make validate
target.
Following you can find the validation results for the APIs you have changed.
API | Status | Request | Response |
---|---|---|---|
connector.check_in |
:green_circle: | 3/3 | 3/3 |
connector.delete |
:green_circle: | 9/9 | 9/9 |
connector.get |
:red_circle: | 60/60 | 59/60 |
connector.last_sync |
:green_circle: | 7/7 | 7/7 |
connector.list |
:green_circle: | 19/19 | 19/19 |
connector.post |
:green_circle: | 7/7 | 7/7 |
connector.put |
:green_circle: | 13/13 | 13/13 |
connector.secret_delete |
:orange_circle: | Missing type | Missing type |
connector.secret_get |
:orange_circle: | Missing type | Missing type |
connector.secret_post |
:orange_circle: | Missing type | Missing type |
connector.secret_put |
:orange_circle: | Missing type | Missing type |
connector.sync_job_cancel |
:green_circle: | 3/3 | 3/3 |
connector.sync_job_check_in |
:orange_circle: | Missing type | Missing type |
connector.sync_job_claim |
:orange_circle: | Missing type | Missing type |
connector.sync_job_delete |
:green_circle: | 4/4 | 4/4 |
connector.sync_job_error |
:orange_circle: | Missing type | Missing type |
connector.sync_job_get |
:green_circle: | 22/22 | 22/22 |
connector.sync_job_list |
:green_circle: | 12/12 | 12/12 |
connector.sync_job_post |
:green_circle: | 50/50 | 50/50 |
connector.sync_job_update_stats |
:orange_circle: | Missing type | Missing type |
connector.update_active_filtering |
:green_circle: | 1/1 | 1/1 |
connector.update_api_key_id |
:green_circle: | 4/4 | 4/4 |
connector.update_configuration |
:red_circle: | 8/9 | 9/9 |
connector.update_error |
:green_circle: | 4/4 | 4/4 |
connector.update_features |
:orange_circle: | Missing type | Missing type |
connector.update_filtering_validation |
:green_circle: | 3/3 | 3/3 |
connector.update_filtering |
:green_circle: | 12/12 | 12/12 |
connector.update_index_name |
:green_circle: | 4/4 | 4/4 |
connector.update_name |
:green_circle: | 4/4 | 4/4 |
connector.update_native |
:green_circle: | 3/3 | 3/3 |
connector.update_pipeline |
:green_circle: | 3/3 | 3/3 |
connector.update_scheduling |
:green_circle: | 3/3 | 3/3 |
connector.update_service_type |
:green_circle: | 2/2 | 2/2 |
connector.update_status |
:green_circle: | 3/3 | 3/3 |
You can validate these APIs yourself by using the make validate
target.
@l-trotta addressed your feedback around sync_cursor
type :)
Following you can find the validation results for the APIs you have changed.
API | Status | Request | Response |
---|---|---|---|
connector.check_in |
:green_circle: | 3/3 | 3/3 |
connector.delete |
:green_circle: | 9/9 | 9/9 |
connector.get |
:red_circle: | 60/60 | 59/60 |
connector.last_sync |
:green_circle: | 7/7 | 7/7 |
connector.list |
:green_circle: | 19/19 | 19/19 |
connector.post |
:green_circle: | 7/7 | 7/7 |
connector.put |
:green_circle: | 13/13 | 13/13 |
connector.secret_delete |
:orange_circle: | Missing type | Missing type |
connector.secret_get |
:orange_circle: | Missing type | Missing type |
connector.secret_post |
:orange_circle: | Missing type | Missing type |
connector.secret_put |
:orange_circle: | Missing type | Missing type |
connector.sync_job_cancel |
:green_circle: | 3/3 | 3/3 |
connector.sync_job_check_in |
:orange_circle: | Missing type | Missing type |
connector.sync_job_claim |
:orange_circle: | Missing type | Missing type |
connector.sync_job_delete |
:green_circle: | 4/4 | 4/4 |
connector.sync_job_error |
:orange_circle: | Missing type | Missing type |
connector.sync_job_get |
:green_circle: | 22/22 | 22/22 |
connector.sync_job_list |
:green_circle: | 12/12 | 12/12 |
connector.sync_job_post |
:green_circle: | 50/50 | 50/50 |
connector.sync_job_update_stats |
:orange_circle: | Missing type | Missing type |
connector.update_active_filtering |
:green_circle: | 1/1 | 1/1 |
connector.update_api_key_id |
:green_circle: | 4/4 | 4/4 |
connector.update_configuration |
:red_circle: | 8/9 | 9/9 |
connector.update_error |
:green_circle: | 4/4 | 4/4 |
connector.update_features |
:orange_circle: | Missing type | Missing type |
connector.update_filtering_validation |
:green_circle: | 3/3 | 3/3 |
connector.update_filtering |
:green_circle: | 12/12 | 12/12 |
connector.update_index_name |
:green_circle: | 4/4 | 4/4 |
connector.update_name |
:green_circle: | 4/4 | 4/4 |
connector.update_native |
:green_circle: | 3/3 | 3/3 |
connector.update_pipeline |
:green_circle: | 3/3 | 3/3 |
connector.update_scheduling |
:green_circle: | 3/3 | 3/3 |
connector.update_service_type |
:green_circle: | 2/2 | 2/2 |
connector.update_status |
:green_circle: | 3/3 | 3/3 |
You can validate these APIs yourself by using the make validate
target.
The backport to 8.15
failed:
The process '/usr/bin/git' failed with exit code 1
To backport manually, run these commands in your terminal:
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-8.15 8.15
# Navigate to the new working tree
cd .worktrees/backport-8.15
# Create a new branch
git switch --create backport-2675-to-8.15
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4b67d9a824878e54a250d4c9c333474a9a06a141
# Push it to GitHub
git push --set-upstream origin backport-2675-to-8.15
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-8.15
Then, create a pull request where the base
branch is 8.15
and the compare
/head
branch is backport-2675-to-8.15
.
Hey π
As discussed, I reviewed the existing Connector API endpoints and added patches to update them to the
8.15
state.Changes
index_name
anderror
(includinglast_sync_error
andlast_access_control_sync_error
) are the only properties that use semantic null.index_name
can be set to null to detach the connector, error can be reset tonull
to indicate healthy state again. We are using optional for everything else.sync_cursor
field in the Connector definition (we missed it before) and add this as an arg to_last_sync
endpoint (new payload field added in 8.15)ConnectorFeatures
representation as it included long-deprecated feature names (filtering_advanced_config
andfiltering_rules
), add missingnative_connector_api_keys
Note
We have this open issue with
_last_sync
endpoint (confusion between semantic null and optional when updatinglast_sync_*_error
). We are looking into fixing this.Also, this PR should address existing endpoints, there are couple of endpoints that are still missing. Let's work on this patch first. :)