search

napalm-automation / napalm-ansible

Apache License 2.0
245 stars 103 forks source link

Create a commit + confirm module #189

Open ktbyers opened 3 years ago

rbcollins123 commented 3 years ago

@ktbyers I could submit an initial implementation of this. Do you have any thoughts on implementation details that you'd like incorporated up-front?

ktbyers commented 3 years ago

@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 commented 3 years ago

@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?

rbcollins123 commented 3 years ago

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.

ktbyers commented 3 years ago

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).

rbcollins123 commented 3 years ago

If we do it innapalm_install_config we can start with the following new args:

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.

ktbyers commented 3 years ago

Yeah, Travis-CI should be eliminated and we should migrate to GH Actions.

Can you explain how confirm_delay would operate?

ktbyers commented 3 years ago

We would also need to implement the following so we had a way to cancel the in-process commit-confirm:

https://github.com/napalm-automation/napalm-ansible/issues/188

rbcollins123 commented 3 years ago

The module would always attempt to confirm the session after confirm_delay seconds

On Sun, Sep 12, 2021 at 8:10 PM Kirk Byers @.***> wrote:

Yeah, Travis-CI should be eliminated and we should migrate to GH Actions.

Can you explain how confirm_delay would operate?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/napalm-automation/napalm-ansible/issues/189#issuecomment-917739726, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2S4CUL5EZGUJWDTIDD7X3UBU6NTANCNFSM44GGL62A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

ktbyers commented 3 years ago

Okay, sounds good, but maybe a different name: auto_confirm_time maybe? I am open to others.

rbcollins123 commented 3 years ago

Makes sense to me!

On Mon, Sep 13, 2021 at 10:11 AM Kirk Byers @.***> wrote:

Okay, sounds good, but maybe a different name: auto_confirm_time maybe? I am open to others.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/napalm-automation/napalm-ansible/issues/189#issuecomment-918233039, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC2S4CWXXKE4KIJQ3V5LC4LUBYA7PANCNFSM44GGL62A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.