Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DO NOT Merge: Add commit confirmed plus commit message support #550

Closed
wants to merge 5 commits into from
Closed

DO NOT Merge: Add commit confirmed plus commit message support #550

wants to merge 5 commits into from

Conversation

ktbyers
Copy link
Contributor

@ktbyers ktbyers commented Nov 22, 2017

No description provided.

@coveralls
Copy link

coveralls commented Nov 22, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.3%) to 77.84% when pulling 0b93f4f on commit_confirmed3 into cd37097 on develop.

@ktbyers
Copy link
Contributor Author

ktbyers commented Nov 22, 2017

Resubmitted due to some merge conflicts...see additional notes here: #544

@ktbyers ktbyers mentioned this pull request Nov 22, 2017
Closed
@mirceaulinic mirceaulinic requested a review from a team November 23, 2017 10:46
@mirceaulinic mirceaulinic added this to the APPROVED milestone Nov 23, 2017
mirceaulinic
mirceaulinic reviewed Dec 15, 2017
View reviewed changes
Copy link
Member

@mirceaulinic mirceaulinic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ktbyers is this ready (at least for IOS)? Junos and IOS-XR support both confirmed and message, I can continue from here if that's fine.

napalm/junos/junos.py
"""Commit configuration."""
if confirmed is not None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junos supports commit confirmed (and message).

napalm/iosxr/iosxr.py
@@ -145,7 +145,10 @@ def compare_config(self):
else:
return self.device.compare_config().strip()

def commit_config(self):
def commit_config(self, confirmed=None, message=None):
if confirmed is not None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IOS-XR too (also message).

@coveralls
Copy link

coveralls commented Dec 15, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.3%) to 78.449% when pulling eb80297 on commit_confirmed3 into 29ee1e5 on develop.

@ktbyers
Copy link
Contributor Author

ktbyers commented Dec 20, 2017

@mirceaulinic Cisco IOS should be ready. I tested that when I implemented it.

@ktbyers
Copy link
Contributor Author

ktbyers commented Dec 20, 2017

@mirceaulinic Let me know when you add this into other platforms and I will regression test it in my lab environment.

We should also regression test it when it gets merged into develop branch (which I should be able to do also).

dbarrosop
dbarrosop reviewed Jan 2, 2018
View reviewed changes
napalm/base/base.py
@@ -178,12 +178,16 @@ def compare_config(self):
"""
raise NotImplementedError

def commit_config(self):
def commit_config(self, confirmed=None, message=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should document what "confirmed" is supposed to be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also really think we shouldn't name this variable confirmed. It is a terrible name; revert_in or timer are way better names.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confirmed is logical and is the most widely adopted name for this function. Basically saying you have to confirm this commit in how many minutes.

I think we will probably just add more confusion by naming it something else.

But agreed...it needs documented.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on revert_in, describes the intent better. confirmed nomenclature maybe better known by network engineers, but I wouldn't cater to them exclusively with this library, you may have engineers without that background.

@dbarrosop
Copy link
Member

I think this whole functionality requires two extra methods:

  1. has_pending_commit() that returns whether if the system has a commit pending confirmation.
  2. revert_commit() basically to shortcircuit the timer and revert immediately.

@ktbyers
Copy link
Contributor Author

ktbyers commented Jan 21, 2018

Added two methods per @dbarrosop comment (although I changed the name of one of them). Implemented and tested them on Cisco IOS.

@dbarrosop
Copy link
Member

Awesome! :)

@ktbyers
Copy link
Contributor Author

ktbyers commented Jan 31, 2018 •
edited
Loading

@dbarrosop @mirceaulinic Did some more research and review (which also helped me remember past research):

  1. IOS - Should be able to do reasonable system-wide solution.

  2. NX-OS - Does not have any reasonable solution (AFAIK). The only thing I see is a kludge that we shouldn't do. This is also what I found in past research. We can ask in Cisco channel or NAPALM channel and see if anyone know of a reasonable solution, but I am not seeing one.

  3. EOS - I think there is support, but a specific version is required. I believe I had this discussion with @bewing previously.

  4. Junos - should be fine

  5. IOS-XR - Can you commit-confirm in a different SSH session on IOS-XR? I wasn't seeing a good way of doing this in brief testing. @mirceaulinic

@dbarrosop
Copy link
Member

We need to confirm 5. @mirceaulinic is that something you can look into?

@ktbyers
Copy link
Contributor Author

ktbyers commented Apr 18, 2019

Closing...don't think it makes sense to keep this PR open any longer.

@ktbyers ktbyers closed this Apr 18, 2019
@rbcollins123 rbcollins123 mentioned this pull request Feb 6, 2020
Open
@ktbyers ktbyers mentioned this pull request Sep 13, 2020
Closed
@DanSheps DanSheps mentioned this pull request Oct 18, 2021
Closed
1 task
@ktbyers ktbyers deleted the commit_confirmed3 branch February 28, 2024 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbarrosop dbarrosop dbarrosop left review comments

@sdanelli sdanelli sdanelli left review comments

@mirceaulinic mirceaulinic mirceaulinic left review comments

Assignees
No one assigned
Labels
api change base core enhancement
Projects
None yet
Milestone
APPROVED
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ktbyers @coveralls @dbarrosop @sdanelli @mirceaulinic