Add artifact version batch request #3164
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
internal
| "different user." | ||
| ) | ||
|
|
||
| verify_permission(resource_type=resource_type, action=Action.CREATE) |
There was a problem hiding this comment.
I guess this is not a use case that we might bump into right now, but I am asking this with the future in mind. When we implement these batch-creation endpoints, what happens if only one entity fails during the creation (due to RBAC or any other reason like EntityExists errors) Should we fail the whole batch or only exclude the failing ones?
There was a problem hiding this comment.
For the creation, we only validate that the type is allowed to be created. I think what you have described can not happen.
There was a problem hiding this comment.
@bcdurak I think the comment on this line is very misleading 😅
As @avishniakov already pointed out, this check is only happening once, and it's happening before any item of the batch is even touched. But I think in general, yes it might happen that item 5 of the batch fails during it's creation.
In that case, we could:
- roll back all previous changes and return a failure for the request
- continue with the next item in the batch, and have the response flexible that it can indicate which items were created and which failed
Currently, we do none of that and simply loop over the batch and once we fail we fail, but keep the already created items in the DB.
I don't think that's ideal, but this PR simply mirrors the previous behaviour for artifact creation at step end, only that it happens with one request. Definitely something to improve on in the future!
There was a problem hiding this comment.
Yeah, I guess I picked to wrong line to leave my comment on 😅 I do understand that the check is happening only once, before everything gets created, but I was focused on what comes after, mainly during the creation of these entities. I think you understood what I meant with that question @schustmi.
I do not have a strong opinion on which one would be the ideal solution (the current one, rollback, or continue) but this might be something that we might want to look into in the future as we generalize the usage of these batch endpoints, that's why I wanted to point it out here.
Mirroring the previous behavior makes sense here though 👍
| "different user." | ||
| ) | ||
|
|
||
| verify_permission(resource_type=resource_type, action=Action.CREATE) |
There was a problem hiding this comment.
For the creation, we only validate that the type is allowed to be created. I think what you have described can not happen.
Describe changes
This PR implements an artifact version batch endpoint that is used to reduce the amount of requests needed to store the output artifacts at the end of a step.
Pre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes