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

Use context manager with Nornir plays #128

Merged
merged 2 commits into from Sep 23, 2021

Conversation

itdependsnetworks
Copy link
Contributor

No description provided.

Copy link
Contributor

@glennmatthews glennmatthews left a comment

Choose a reason for hiding this comment

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

Changing to context managers makes sense to me and seems like a logical improvement. I noticed with this change you are now reraising the original error instead of raising a new NornirNautobotException() - just checking whether that's an intentional change, and what if any consequences does that change have? (I.e., does whatever is calling these functions need to change from an except NornirNautobotException to an except Exception, things like that?)

@itdependsnetworks
Copy link
Contributor Author

It was a purposeful thing, upon thinking about it, why wouldn't we want to log the message no matter the error. This seemed like a logical way to do it. There may be some situations where you get multiple messages, but I thought it would be an overall improvement.

@itdependsnetworks itdependsnetworks merged commit eeadb24 into nautobot:develop Sep 23, 2021
@racsoce
Copy link

racsoce commented Sep 24, 2021

Thanks, this also solves the issue with leaving open ssh connections to the devices while doing the Backup Configurations by doing the close_connections() by the context, I just tested and is working good 👍
I was writing the Issue recommending the line nornir_obj.close_connections(nornir_obj.inventory.hosts) but this is the best fix 👍 :-)

@itdependsnetworks
Copy link
Contributor Author

itdependsnetworks commented Sep 24, 2021

Yea, I should have been more clear that this was the intention of the PR, and should probably follow up with some documentation on the potential issue.

@itdependsnetworks itdependsnetworks linked an issue Sep 24, 2021 that may be closed by this pull request
@itdependsnetworks itdependsnetworks deleted the context-manager branch September 24, 2021 15:14
jmpettit pushed a commit to jmpettit/nautobot-app-golden-config that referenced this pull request Jan 30, 2024
* Use context manager with Nornir plays
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.

SSH Sessions does not die
3 participants