Create a commit + confirm module
ktbyers opened this issue · 11 comments
@ktbyers I could submit an initial implementation of this. Do you have any thoughts on implementation details that you'd like incorporated up-front?
@rbcollins123 I haven't really thought about, but I would just look at the key design arguments in commit-confirmed and then map those to Ansible arguments so that all of the key features could be used. I have a video series on commit-confirm if it helps (here is the first video: https://youtu.be/wTQ_LuYZC68)
@rbcollins123 I haven't really thought about, but I would just look at the key design arguments in commit-confirmed and then map those to Ansible arguments so that all of the key features could be used. I have a video series on commit-confirm if it helps (here is the first video: https://youtu.be/wTQ_LuYZC68)
Oh yeah, I'm very familiar with it from when we all worked through this issue a while back, but I was more curious if your desire was to have napalm_install_config modified to handle it via new args or if the title of this issue was indeed indicative of doing a new napalm_install_config_withconfirm type module?
Looks like the test-suite of napalm-ansible relies upon the MockDriver, but that didn't have support for the confirm_commit API changes, so I posted this PR to get that added downstream.
I haven't really thought about it much. We maybe should just sketch out what it would look like in both cases? In other words, if we overload napalm_install_config what arguments are we adding?
I doubt having it in the MockDriver tests matter (i.e. I definitely wouldn't gate creating an Ansible module on this). The MockDriver tests for config operations are not worth very much. In other words, they don't really tell you much regarding whether the config operations actually work or not.
I haven't tested any config operations using the MockDriver in probably 3+ years (and I probably tested the config operations as much anyone).
If we do it innapalm_install_config we can start with the following new args:
- confirm_commit: boolean, default=false, set to true to cause (if we do a new module this one can be dropped and the rest will be the same)
- revert_in: int, default=None, set to # of seconds desired for configuration session timer confirmation window
- confirm_delay: int, default=0, set to the # of seconds to wait before the module attempts to confirm the configuration session. This can be useful if users want to configure a convergence/quiescent period before confirming the session. We can allow the user full control here with no data validation, or we could force that values must be < revert_in.
I cloned napalm_install_config over into napalm_install_config_confirm on a fork HERE to show a new module flavor of it. If that's easier to see/review I could fire a PR for that, but personally, I like adding it into the current module more since there is a ton of code duplication that we can avoid.
Since implementations of confirm_commit vary by underlying Napalm driver, it could be useful for the Ansible module to check if the dev_os being called by the napalm-ansible module can actually support the use of confirm_commit if a user puts the args in an Ansible play. We could either warn or fail the module if a NotImplmentedError was returned. If that behavior is in the module, we'd need that API to at least function in MockDriver given the current test-suite in napalm-ansible wouldn't we? Example HERE
I also took the liberty of porting the Travis config into a GitHub Actions workflow file on that fork if there's any desire to move to that (I think I remember napalm did a while back?). I left out the deploy to PyPi steps for now but can port that also if you want in the same/diff PR when we get to that step.
Yeah, Travis-CI should be eliminated and we should migrate to GH Actions.
Can you explain how confirm_delay would operate?
We would also need to implement the following so we had a way to cancel the in-process commit-confirm:
Okay, sounds good, but maybe a different name: auto_confirm_time maybe? I am open to others.