-
Notifications
You must be signed in to change notification settings - Fork 23
feat(specs): add put and delete API for compositions and rules #5282
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
✔️ Code generated!
📊 Benchmark resultsBenchmarks performed on the method using a mock server, the results might not reflect the real-world performance.
|
bffbc7c
to
72ff44d
Compare
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 great! feel free to ping me when I can merge
ad30527
to
b2d864a
Compare
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.
Nice !
Thanks 👍
Ready to merge :) |
'404': | ||
$ref: '../../../common/responses/CompositionNotFound.yml' |
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.
'404': | |
$ref: '../../../common/responses/CompositionNotFound.yml' |
Same as for Composition Rules, we don't actually check the Composition exists and always just send the jobs to delete it 🤷
https://github.com/algolia/metis/blob/7ef740ee88b0cea24d2ac51045b8c77060025cd8/modules/services/api/internal/compositions/management.go#L198-L227
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.
Updated, thanks
This is ready again, thanks for your patience :) |
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 have a comment about the return type of the delete endpoint, sorry this is beyond the scope of the api client
content: | ||
application/json: | ||
schema: | ||
$ref: '../../../common/responses/common.yml#/taskID' |
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.
does this endpoint return the taskID directly ? what do you think about wrapping the response in a object like {"taskID": number}
? This way it's more future proof IMO, if you need to add a deletedAt
or something else.
All the other endpoint returning a taskID wrap it in a object btw
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 for catching this, indeed the response is {"taskID": <int>}
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.
rebased and pushed up fix
d9ce2b3
to
3ece02d
Compare
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 !
… (generated) [skip ci] Co-authored-by: Ben Kalmus <[email protected]>
…ated) algolia/api-clients-automation#5282 Co-authored-by: algolia-bot <[email protected]> Co-authored-by: Ben Kalmus <[email protected]>
🧭 What and Why
🎟 JIRA Ticket:
Changes included:
/1/compositions/{compositionID}/
for Compositions/1/compositions/{compositionID}/rules/{ruleID}
for Composition Rules🧪 Test
CI