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

Think through 2.x style run() implementation and best practice #272

Open
cardoe opened this issue Nov 8, 2023 · 1 comment
Open

Think through 2.x style run() implementation and best practice #272

cardoe opened this issue Nov 8, 2023 · 1 comment
Labels
status: internal review Internal discussion is required to move forward with issue type: documentation Issues/PRs addressing documentation.

Comments

@cardoe
Copy link

cardoe commented Nov 8, 2023

A bit of a follow on to #153 and related to #183 as well as nautobot/nautobot#4761, I think some thought needs to be put into how this plugin documents and defines the best practice for implementing run().

One of the issues with the current style (example of which is https://github.com/nautobot/nautobot-plugin-ssot/blob/35803947ddd8f5c44bf9bbf0df2a91398e9c5539/nautobot_ssot/integrations/infoblox/jobs.py#L60-L65 ) is the fact that no type checker will be happy with that. The syntax used is the PEP484 type annotation syntax and the self.debug value should be of type BooleanVar but instead when run() is called it's of type bool.

Further down the path of type checking, it will be impossible to properly put type annotations inside of the Adapter implementations because of the circular reference that will cause from https://github.com/nautobot/nautobot-plugin-ssot/blob/35803947ddd8f5c44bf9bbf0df2a91398e9c5539/nautobot_ssot/integrations/infoblox/jobs.py#L49 where job=self is being passed to the Adapter. There would be no way to import the Job into the Adapter implementation to use as a type annotation. This is because the current recommended code style involves putting the Job in one file and the Adapter in another.

There's no current way to break this because there's no way to save variables from run() to be passed to the load_source_adapter() and load_target_adapter() methods.

I would recommend moving away from the passing of the job down to the Adapter themselves in the long run and coming up with a way to pass the variables. Most of the implementations today just pass the job down for the logger only so potentially pass the logger down or just have implementers import the get_task_logger(). Looking at the code of the plugin today there's a few ways to import the logger used and even looking at the current Nautobot 2.x docs it suggests multiple ways to import the logger.

@cardoe
Copy link
Author

cardoe commented Nov 8, 2023

@gsnider2195 I created this based on our convo on nautobot/nautobot#4761

@jdrew82 jdrew82 added status: internal review Internal discussion is required to move forward with issue type: documentation Issues/PRs addressing documentation. labels Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: internal review Internal discussion is required to move forward with issue type: documentation Issues/PRs addressing documentation.
Projects
None yet
Development

No branches or pull requests

2 participants