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

Improve hot-reloading of custom scripts #7803

Closed
kkthxbye-code opened this issue Nov 11, 2021 · 2 comments · Fixed by #7820
Closed

Improve hot-reloading of custom scripts #7803

kkthxbye-code opened this issue Nov 11, 2021 · 2 comments · Fixed by #7820
Assignees
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application

Comments

@kkthxbye-code
Copy link
Contributor

NetBox version

V3.0.9

Feature type

Change to existing functionality

Proposed functionality

Allow renaming of classes in custom scripts. Currently if you rename a class or remove a class from a file, netbox will throw OSError : could not find class definition and the script functionality will be broken until the application server is restarted.

The reason for this is that python uses sys.modules to cache all imported modules. When you change a class in the script and reload it in get_scripts, which is called on both the list view and when executing scripts, the module includes both the old name for the class and the new name (not sure why), causing the import to fail on the old name.

The solution is pretty simple, deleting the module from sys.modules. A simple implementation can be seen here:

kkthxbye-code@ae6ed97

I wasn't able to notice any performance regressions, the extra load should be minimal even with a lot of scripts. The advantage is that the loaded script is actually correct.

Use case

Scripts already allow changing code without restart, with the requested change you will also be able to rename or delete script classes.

Database changes

None

External dependencies

None

@kkthxbye-code kkthxbye-code added the type: feature Introduction of new functionality to the application label Nov 11, 2021
@kkthxbye-code
Copy link
Contributor Author

Note: I wasn't sure if this constituted a bug report or a feature request. Scripts are made to be hot reloaded as far as I can tell, so it would make sense to fix this as a bug.

Related old tickets: #3579 - #1590

@jeremystretch jeremystretch added the status: accepted This issue has been accepted for implementation label Nov 11, 2021
@jeremystretch
Copy link
Member

Nice, makes sense to me. @kkthxbye-code assigned this to you for a PR to merge your changes. Thanks!

jeremystretch added a commit that referenced this issue Nov 12, 2021
Fix #7803: Clear sys.modules cache when reloading scripts
jeremystretch added a commit that referenced this issue Nov 12, 2021
jlixfeld added a commit to jlixfeld/nautobot that referenced this issue Dec 8, 2021
Port of fix for the same issue: netbox-community/netbox#7803
glennmatthews pushed a commit to nautobot/nautobot that referenced this issue Dec 15, 2021
Port of fix for the same issue: netbox-community/netbox#7803
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants