Skip to content

Connector SyncJob API spec #2503

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

Merged
merged 10 commits into from
Apr 22, 2024
Merged

Connector SyncJob API spec #2503

merged 10 commits into from
Apr 22, 2024

Conversation

jedrazb
Copy link
Member

@jedrazb jedrazb commented Apr 11, 2024

Changes

Added the API spec for endpoints related to connector sync job managements.

@jedrazb jedrazb requested a review from a team as a code owner April 11, 2024 12:14
id: Id
indexed_document_count: long
indexed_document_volume: long
job_type: SyncJobType
Copy link
Contributor

Choose a reason for hiding this comment

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

can job_type be a SyncJobType or SyncJobType[] enum consistently?

Copy link
Contributor

Choose a reason for hiding this comment

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

same for trigger_method

Copy link
Member Author

Choose a reason for hiding this comment

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

In a ConnectorSyncJob representation they are not an array, there is singe value associated with trigger_method and job_type

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is, in this class job_type and trigger_method are enums, while in the classes SyncJobPostRequest.ts and SyncJobListRequest.ts they are Name/Names, which are just strings, can't they be the same enum everywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh I see what you mean! For job_type this is passed as a comma-separated list in the URL param of SyncJobListRequest. I was not sure if defining this as SyncJobType[] would work with URL param list, and I saw Names in other examples of requests that pass comma separated lists in url params.

If the usage of SyncJobType[] and SyncJobTriggerMethod is correct, I can adapt this in the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks :) I think it's more coherent this way from the clients perspective, to always provide enums where there's fixed values

Copy link
Member Author

Choose a reason for hiding this comment

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

I addressed that feedback in the updated PR :)

@l-trotta
Copy link
Contributor

I have found a type that was introduced in the previous PR that I'm only now noticing is not matching the server code. Since this new API is using it, could you fix it here?
FilteringConfig -> active and draft should be optional, domain should be removed, it seems like an internal parameter

Copy link
Contributor

@Anaethelion Anaethelion left a comment

Choose a reason for hiding this comment

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

Left a few comments, found some fields that could be hinted as datetime to enhance deserialization.

@jedrazb
Copy link
Member Author

jedrazb commented Apr 17, 2024

I have found a type that was introduced in the previous PR that I'm only now noticing is not matching the server code. Since this new API is using it, could you fix it here?
FilteringConfig -> active and draft should be optional, domain should be removed, it seems like an internal parameter

Yup, there were couple of changes to connector (not sync job) endpoints in 8.14 that were merged after we merged #2367 . I'm planning to followup to adapt the types in a separate PR.

@jedrazb
Copy link
Member Author

jedrazb commented Apr 17, 2024

Sync job endpoints were moved under the connector namespace, I reflected changes in this PR after elastic/elasticsearch#107476 was merged.

Also, I changed Name to more specific allowed enum types (SyncJobTriggerMethod and SyncJobType) in sync_job_post request

@jedrazb jedrazb requested review from l-trotta and Anaethelion April 17, 2024 09:32
@jedrazb jedrazb requested a review from l-trotta April 18, 2024 09:00
Copy link
Contributor

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

API Status Request Response
connector.check_in Missing test Missing test
connector.delete Missing test Missing test
connector.get Missing test Missing test
connector.last_sync Missing test Missing test
connector.list Missing test Missing test
connector.post Missing test Missing test
connector.put Missing test Missing test
connector.sync_job_cancel Missing test Missing test
connector.sync_job_delete Missing test Missing test
connector.sync_job_get Missing test Missing test
connector.sync_job_list Missing test Missing test
connector.sync_job_post Missing test Missing test
connector.update_api_key_id Missing test Missing test
connector.update_configuration Missing test Missing test
connector.update_error Missing test Missing test
connector.update_filtering Missing test Missing test
connector.update_index_name Missing test Missing test
connector.update_name Missing test Missing test
connector.update_native Missing test Missing test
connector.update_pipeline Missing test Missing test
connector.update_scheduling Missing test Missing test
connector.update_service_type Missing test Missing test
connector.update_status Missing test Missing test

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

@jedrazb
Copy link
Member Author

jedrazb commented Apr 18, 2024

Thank you all for reviewing! I need to include this in the 8.14 release. I still don't see the backport 8.14 label, do I need to do sth specific to make sure it lands in the 8.14 BC?

Copy link
Contributor

@Anaethelion Anaethelion left a comment

Choose a reason for hiding this comment

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

LGTM!

@jedrazb jedrazb merged commit 9f0dc3d into main Apr 22, 2024
7 checks passed
@jedrazb jedrazb deleted the add-connector-sync-job-api-spec branch April 22, 2024 12:55
Copy link
Contributor

The backport to 8.14 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.14 8.14
# Navigate to the new working tree
cd .worktrees/backport-8.14
# Create a new branch
git switch --create backport-2503-to-8.14
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 9f0dc3db0a0f82ef9ebebb276bcfdcaf28c69e6a
# Push it to GitHub
git push --set-upstream origin backport-2503-to-8.14
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-8.14

Then, create a pull request where the base branch is 8.14 and the compare/head branch is backport-2503-to-8.14.

jedrazb added a commit that referenced this pull request Apr 23, 2024
(cherry picked from commit 9f0dc3d)
l-trotta pushed a commit that referenced this pull request Apr 23, 2024
* Connector SyncJob API spec (#2503)

(cherry picked from commit 9f0dc3d)

* Run make contrib
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