-
Notifications
You must be signed in to change notification settings - Fork 98
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
Conversation
id: Id | ||
indexed_document_count: long | ||
indexed_document_volume: long | ||
job_type: SyncJobType |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for trigger_method
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
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? |
There was a problem hiding this 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.
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. |
Sync job endpoints were moved under the Also, I changed |
Following you can find the validation results for the APIs you have changed.
You can validate these APIs yourself by using the |
Thank you all for reviewing! I need to include this in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The backport to
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 |
(cherry picked from commit 9f0dc3d)
Changes
Added the API spec for endpoints related to connector sync job managements.