nautobot / nautobot-app-ssot

Single Source of Truth for Nautobot
https://docs.nautobot.com/projects/ssot/en/latest/
Other
38 stars 41 forks source link

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

Open cardoe opened 1 year ago

cardoe commented 1 year ago

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 commented 1 year ago

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