-
Notifications
You must be signed in to change notification settings - Fork 65
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
device_templates.attach_to_template polls w/ utils.waitfor_action_completion #97
Comments
Just for a little background on this repo, we have not taken the view that the SDK should simply be Python bindings for the vManage API, rather that it is OK to put additional logic in places where it makes sense. That said, this repo also houses the CLI and the Ansible modules and they are probably the most widely used abstractions of the SDK. The poll-for-action-completion logic could be moved there instead (and would need to be completed and tested for any PR that changes the attach logic due to the way we version things at the moment.) On the other hand, will it be a common use case that we would want to attach a device to a template and not know whether it completed or not? I don't really know the answer to that and could certainly be convinced that it would be a common use case. In our use case (primarily automation for DevOps CI/CD) it is not something we need. |
I guess I want a snappy API call that doesn't block. I'm using this in a function passed to a Django-RQ worker. The default rqworker timeout on Netbox where I'm using this is 300 seconds. Most of the time the actions on VManage complete in that time, but I don't want to depend on it and not get the actionId back so I can easily go check it later. Once I have the actionId, an RQ-Scheduler job runs periodically to pull the status of any outstanding actions (on DNA Center or Vmanage). If no one else has that need, I can just modify it before I install it. It's a small change. |
What about just adding a new parameter |
That approach works for me. If we do this PR, we should probably look at all areas |
Sure, I'll look through and find the other spots it is used and add wait to those as well. I also need to be able to query the actionId's status on demand. I could add that function to utilities alongside the current waitfor_action_completion. Would you guys rather have that in a separate PR? |
I'm OK with it being in the same PR because it is something we should have for when |
I'm with @jasonking3 - the changes are pretty well related, and I don't think it would make the review unwieldy to have it all lumped together. |
Ok, I found several to add wait to but found these 3 where it doesn't make sense:
There are the spots I am adding it:
Then add a function called get_action_status that returns the same data structure as waitfor_action_completion, but instead of looping will just return whatever it gets on the first request. Let me know if that sounds good and I'll test the changes and then submit a PR. |
This sounds reasonable to me. It makes sense that the only place we should need this is in the api methods. I'm fine with the |
I think the SDK is trying to do too much here. Instead of providing a wrapper for the API it is building in logic that the user may or may not actually want. Would anyone be opposed if I submitted a pull request to remove the waitfor_action_completion from this function and just return the actionId?
Then if someone wants to know the status of the actionId they can request it, loop it, or whatever in their own code.
The text was updated successfully, but these errors were encountered: