-
Notifications
You must be signed in to change notification settings - Fork 115
Add spec for project routing CRUD REST API endpoints #5860
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
|
Following you can find the validation changes against the target branch for the APIs.
You can validate these APIs yourself by using the |
flobernd
left a comment
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.
Looks good, thank you 🙂 Just a few minor questions/comments.
| * under the License. | ||
| */ | ||
|
|
||
| export class ProjectRoutingExpression { |
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.
Is this class named the same in the Elasticsearch code base? I personally would have used ProjectRouting as the class name since the expression is only a part of it, but if it's the same in Elasticsearch, let's keep it as is 🙂
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'm looking at the server code (in serverless) and it seems like every class there is actually called SomethingProjectRoutingExpression, so I'd actually argue all other endpoint classes should be named as closely as possible to what's in the server code
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 the server code it is SavedProjectRoutingExpression, but I think Saved prefix would be confusing in API
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.
agree for Saved, I'd keep Expression though
specification/project_routing/create_single/CreateRoutingRequest.ts
Outdated
Show resolved
Hide resolved
flobernd
left a comment
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! Thanks a lot!
Elasticsearch PR: elastic/elasticsearch#139634