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.

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

Add commit comment feature. (#40 - #43) #20

Merged
merged 2 commits into from
Mar 23, 2022

Conversation

gcasella
Copy link
Contributor

This commit change is to add the comment functionality for pushing changes out to network devices.

)

This commit change is to add the comment functionality for pushing changes out to network devices.
@@ -36,7 +37,10 @@ def napalm_configure(

dry_run = task.is_dry_run(dry_run)
if not dry_run and diff:
device.commit_config()
if comment:
Copy link
Contributor

@dbarrosop dbarrosop Mar 22, 2022

Choose a reason for hiding this comment

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

Based on napalm's documentation I think you should be able to just do the following:

...
comment: str = "",
...

if not dry_run and diff:
    device.commit_config(message=comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is true, however i like to put that logic in there to see if a commit_message actually exists prior to sending it in.

@@ -11,6 +11,7 @@ def napalm_configure(
filename: Optional[str] = None,
configuration: Optional[str] = None,
replace: bool = False,
comment: str = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe commit_message is a bit more descriptive argument name

@dbarrosop
Copy link
Contributor

Thanks for your contribution, I left a couple of very minor comments

1 similar comment
@dbarrosop
Copy link
Contributor

Thanks for your contribution, I left a couple of very minor comments

@gcasella
Copy link
Contributor Author

@dbarrosop absolutely fine with having it as commit_message instead of just comment

Modified `commit` argument with `commit_message` and modified line 41 to reflect this.
@gcasella
Copy link
Contributor Author

I've added the slight recommendations to the parameter naming which i agree are a bit more descriptive.

@dbarrosop dbarrosop merged commit f3c85d7 into nornir-automation:master Mar 23, 2022
@dbarrosop
Copy link
Contributor

Thanks!

@gcasella gcasella deleted the add_commit_comment branch March 23, 2022 15:54
@gcasella
Copy link
Contributor Author

Have a question. At what point would this change be made when using the pip install nornir_napalm command?

@gcasella
Copy link
Contributor Author

@dbarrosop regarding my previous comment. I ask because when I install salt-nornir / nornir-salt it's taking the latest version via pip

Thx in advance

@dbarrosop
Copy link
Contributor

I will try to do a quick release tomorrow

@dbarrosop
Copy link
Contributor

That might take longer than expected, somehow the CI broke and I don't have the cycles right now to fix it:

#21

If someone can help out with that it will certainly expedite the release.

@gcasella
Copy link
Contributor Author

@dbarrosop it looks like it's related to the installation of pyzmq.
Not sure how the pipeline gets executed or what it's executing specifically, but looks like it runs an Ubuntu 20.x image, that image should have libzmq3-dev installed using apt

As for the other failures it looks like it never finished off running linters to install all required packages the other ones just show ruamel is not installed

Not sure if any of this helps you but that was from my quick glance at the failure of the pipeline.

@ubaumann
Copy link
Contributor

I took a quick look and I think the problem is the linter is running with Python 3.10 and for the pyzmq version 19.0.1 is no whl file on mypy for python 3.10 and pip downloads the source and tries to build it and GCC failes.

I created an PR to update all the dependencies #22

@dbarrosop
Copy link
Contributor

@gcasella nornir_napalm 0.2.0 has been released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants