Skip to content

Conversation

@kkthxbye-code
Copy link
Contributor

Fixes: #10258

The purpose of this FR is to allow a structure like this:

scripts
├── __init__.py
├── script0.py
└── testscriptfolder
    ├── __init__.py
    ├── script1.py
    ├── script2.py
    └── testnestedfolder
        ├── __init__.py
        └── script3.py

All scripts contain (with the numbers replaced):

from extras.scripts import *

class Script0(Script):
    def run(self, data, commit):
        self.log_success("Script0")

The __init__.py file in the testscriptfolder contains:

from .script1 import Script1
from .script2 import Script2
from .testnestedfolder.script3 import Script3

This setup results in the following script list with the PR:

image

This is achieved by splitting import paths so that the module_name contains the base module only, while the script name contains the rest of the path. For Script3 the import path would look like:

testscriptfolder.testnestedfolder.script3 which would be split to:

module_name = "testscriptfolder"
script_name = "testnestedfolder.script3"

The advantage is that scripts become much easier to manage as before the only way to have scripts grouped under the same module was to have them all in the same file. With long scripts this instantly becomes unwieldy.

I tested every combination I could come up with, and as far as I can tell defining custom names for the scripts still work. Defining custom names for the module still works (done in the __init__.py file). Calling scripts from the API still works. Script ordering still works.

I'm not sure if this is the right approach. As far as I can tell it doesn't break anything, but it's hard to guarantee that people haven't found crazy ways around the limitations of script loading. I would really like some feedback and help testing here. Maybe there's a better way to achieve better script organization that I'm missing.

Another limitation is that I haven't touched reports here.

@jeremystretch
Copy link
Member

LGTM! Thanks for sorting this out @kkthxbye-code.

Another limitation is that I haven't touched reports here.

Do we want to expand the scope of #10258 to cover reports as well, and replicate the necessary changes in this PR, or treat that work as a separate issue?

@kkthxbye-code
Copy link
Contributor Author

@jeremystretch - We might as well get it all fixed. I'll investigate if there are any blockers for just doing the same fixes for reports, if not I'll amend the issue and PR to include that as well. I'll ping you when I got it all figured out.

@kkthxbye-code
Copy link
Contributor Author

I think this is good to go @jeremystretch unless I missed something. I really tried to thoroughly test everything, but there's a lot of leeway with regards to creating scripts. If you have some scripts/reports lying around, feel free to test it out quick before merge.

Copy link
Member

@jeremystretch jeremystretch left a comment

Choose a reason for hiding this comment

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

Haven't found any issues loading either script or reports using the new approach. Thanks!

@jeremystretch jeremystretch merged commit 7b4f525 into netbox-community:develop Sep 15, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Not possible to use custom scripts in nested packages

2 participants