-
Notifications
You must be signed in to change notification settings - Fork 578
DO NOT Merge: Add commit confirmed plus commit message support #550
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
Closed
Closed
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
56324e4
Add framework to support commit confirmed
ktbyers 34bbc19
Cisco IOS proof of concept on commit confirmed
ktbyers 0b93f4f
Adding message argument support
ktbyers eb80297
Merge branch 'develop' into commit_confirmed3
mirceaulinic b923e6f
Add new methods for immediate revert and checking if pending commit c…
ktbyers File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IOS-XR too (also message). |
||
| raise NotImplementedError | ||
|
|
||
| if self.replace: | ||
| self.device.commit_replace_config() | ||
| else: | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -234,8 +234,11 @@ def compare_config(self): | |
| else: | ||
| return diff.strip() | ||
|
|
||
| def commit_config(self): | ||
| def commit_config(self, confirmed=None, message=None): | ||
| """Commit configuration.""" | ||
| if confirmed is not None: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Junos supports commit confirmed (and message). |
||
| raise NotImplementedError | ||
|
|
||
| self.device.cu.commit(ignore_warning=self.ignore_warning) | ||
| if not self.config_lock: | ||
| self._unlock() | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_inortimerare way better names.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confirmedis logical and is the most widely adopted name for this function. Basically saying you have toconfirmthis commit in how many minutes.I think we will probably just add more confusion by naming it something else.
But agreed...it needs documented.
There was a problem hiding this comment.
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.confirmednomenclature maybe better known by network engineers, but I wouldn't cater to them exclusively with this library, you may have engineers without that background.