-
Notifications
You must be signed in to change notification settings - Fork 71
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
Adding a new feature to use commit timer #416
base: main
Are you sure you want to change the base?
Adding a new feature to use commit timer #416
Conversation
Whall/commit timer
Enforcing commit timer "0 < value < 24 hours"
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.
This should be merged
This fixes #375 |
I think this is failing the tests because you only implemented support for the timer in just the HTTP API class. I think you also need to add it to the CLI class in |
@veyloster if you are available I would like to connect and discuss. I am on the ansiblenetwork.slack.com slack |
He's on leave until next Tuesday - happy to help if you need |
I have opened veyloster#5 in your fork |
Add timer suport to the cliconf plugin
for more information, see https://pre-commit.ci
Looks like I goofed on my PR, let me fix |
Failure is due to referencing self in non class function |
for more information, see https://pre-commit.ci
…os into whall/fix_parse_timer
for more information, see https://pre-commit.ci
Whall/fix parse timer
@@ -193,7 +193,9 @@ def edit_config( | |||
commit=True, | |||
replace=None, | |||
comment=None, | |||
**kwargs, |
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.
my only concern around this change is the fact that the only keyword argument that exists is timer - and i think i would rather refactor all the calls to edit_config
rather than introduce kwargs
to this function.
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 agree it's a bit ugly, but if we change the interface and deviate from CliconfBase
, then pylint fails. Not sure if the object instance has any state that contains the timer details?
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 asked @Qalthos on Slack and they replied with a bit of insight:
https://ansiblenetwork.slack.com/archives/CEXQ0U2RH/p1689792762886269
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.
Looks like we might be able to do this in edit_config
timer = self.get_option("timer")
But not sure how to test this specific code path.
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.
ok, I will try out that API call and make a PR to update it, if successful
@@ -402,6 +423,7 @@ def main(): | |||
diff_ignore_lines=dict(type="list", elements="str"), | |||
running_config=dict(aliases=["config"]), | |||
intended_config=dict(), | |||
timer=dict(), |
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.
this needs a type definition, and if required default value.
timer=dict(type="str" default="")
@@ -171,7 +178,8 @@ def load_config(self, commands, commit=False, replace=False): | |||
"""Loads the config commands onto the remote device""" | |||
conn = self._get_connection() | |||
try: | |||
response = conn.edit_config(commands, commit, replace) | |||
timer = parse_timer(self._module) | |||
response = conn.edit_config(commands, commit, replace, None, timer) |
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.
response = conn.edit_config(commands, commit, replace, None, timer) | |
response = conn.edit_config(commands, commit, replace,None, timer=timer) |
@@ -491,6 +504,35 @@ def get_capabilities(self): | |||
return json.loads(capabilities) | |||
|
|||
|
|||
def parse_timer(module): |
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.
Better place for this would be utils.
@veyloster thanks for the work on this feature, I would like to bring more inputs to this, |
SUMMARY
This is about adding a new feature to use commit timer when deploying new config.
Right now there's no option to use it and this is a good mechanism to rollback if a bad configuration is pushed.
ISSUE TYPE
ADDITIONAL INFORMATION
Adding a new timer field in the eos_config module which let you set how long you want it to be (Ex: 1m, 30s ...)
You then need to combine this with another task to commit the ansible session.
Other task can be added in between like doing an ssh reset to make sure the device is still reachable after getting the new config.