[ignore] Add Bulk API handlers for create and delete operations#223
[ignore] Add Bulk API handlers for create and delete operations#223gmicol wants to merge 4 commits intoCiscoDevNet:nd42_integrationfrom
Conversation
gmicol
requested review from
akinross,
allenrobel,
anvitha-jain,
lhercot,
mikewiebe,
mtarking,
sajagana,
samiib and
shrsr
as code owners
… Add Exception for bulk operations.
…ation for these fields.
gmicol
force-pushed
the
nd42_add_bulk_operations
branch
from
11b6d37 to
6e07737
Compare
|
@gmicol Are there any changes required to modules that are using this infrastructure currently without bulk APIs? Is it backwards compatible? |
|
@mikewiebe there is no change needed to modules that are using this infrastructure currently without bulk APIs. I made sure it was backwards compatible. |
allenrobel
left a comment
allenrobel
left a comment
There was a problem hiding this comment.
Similar to @mikewiebe, would be good for reviewers if there were a more detailed description of the changes and design decisions in the PR.
|
|
||
| @requires_bulk_support("supports_bulk_create") | ||
| def create_bulk(self, model_instances: List[ModelType], **kwargs) -> ResponseType: | ||
| pass |
There was a problem hiding this comment.
Since subclasses are expected to implement these, should we consider raising NotImplementedError rather than pass for create_bulk and delete_bulk? This way, if they are called, but not implemented, we'll catch it right away.
There was a problem hiding this comment.
That's a great idea! I will update these methods
| items_to_create_bulk.append(final_item) | ||
| else: | ||
| self.model_orchestrator.create(final_item) | ||
| self.sent.add(final_item) |
There was a problem hiding this comment.
What's the expected behavior if create_bulk() fails and ignore_errors is True? Are the items in self.sent considered sent and would still appear in the output? Wondering if self.sent.add() should be moved to after the bulk call succeeds?
There was a problem hiding this comment.
You are right, in the case of a bulk operation when ignore_errors is True, the items would still appear in self.sent in the output even though the request failed. self.sent.add() should be moved after the bulk call succeeds.
| self.model_orchestrator.delete(item) | ||
|
|
||
| # Remove from collection | ||
| self.existing.delete(identifier) |
There was a problem hiding this comment.
Same comment as with self.sent.add() being called before create_bulk. What is the expected behavior when ignore_errors is True and the delete_bulk call fails?
| for proposed_item in self.proposed: | ||
| try: | ||
| identifier = proposed_item.get_identifier_value() | ||
| identifier = proposed_item.get_identifier_value() |
There was a problem hiding this comment.
What's the expected behavior if get_identifier_value raises and ignore_errors is True? Previously, this was wrapped in a try/except with the except branch containing a conditional for ignore_errors.
There was a problem hiding this comment.
The error would not be catch correctly, will add it back to the try/except
| # Configuration | ||
| self.model_orchestrator = model_orchestrator(sender=self.nd_module) | ||
| # Accept either an orchestrator instance or a class. | ||
| if isinstance(model_orchestrator, type): |
There was a problem hiding this comment.
Nit, but anything that's a class will pass this check, not just orchestrator classes. Fine as long as folks are careful.
There was a problem hiding this comment.
You definitely right. Therefore, I will make the condition more specific to avoid any mistake by devs on the orchestrator class type
| raise NDStateMachineError(error_msg) from e | ||
|
|
||
| # Log deletion | ||
| self.output.assign(after=self.existing) |
There was a problem hiding this comment.
Is this needed given the same on line 139? Just asking since I'm not sure.
There was a problem hiding this comment.
It is needed as this is for delete operation output (_manage_override_deletions and _manage_delete_state methods), whereas line 139 is for saving _manage_create_update_state() output.
But you could argue that this repetitive code that could be set directly in the broader mnage_state function.
4 participants
No description provided.