nornir-automation / nornir

Pluggable multi-threaded framework with inventory management to help operate collections of devices
https://nornir.readthedocs.io/
Apache License 2.0
1.39k stars 235 forks source link

Add property for optional runner #913

Closed ogenstad closed 3 months ago

ogenstad commented 3 months ago

The runner argument to Nornir is defined as being optional but we later use it as if it's always set. Adding a property that returns a RunnerPlugin or raises an error, with this we can also remove the strict_optional = False bypass in the mypy config.

dbarrosop commented 3 months ago

I wonder if it's better to change the signature instead

ogenstad commented 3 months ago

That would be cleaner for sure, I didn't want to change anything that could potentially break someones workflow. I don't know if it would be an issue but potentially someone could do something like this now:

nr = Nornir()
my_nr = nr.with_runner(runner=xyz)

My guess is that most people would be using InitNornir where the runner is always set but I don't know if this would be considered enough of an issue?

Alternatively load_runner could be moved so that it's called here again if runner isn't set.

self.runner = runner or load_runner(self.config)
dbarrosop commented 3 months ago

I honestly would consider this a bug. It's also a typing issue so it will only break linters/development, it shouldn't break the runtime, should it?

ogenstad commented 3 months ago

It's a typing issue but depending on how it's solved it could be problematic in some use cases.

    def __init__(
        self,
        inventory: Inventory,
        config: Optional[Config] = None,
        data: Optional[GlobalState] = None,
        processors: Optional[Processors] = None,
        runner: Optional[RunnerPlugin] = None,
    ) -> None:
        self.data = data if data is not None else GlobalState()
        self.inventory = inventory
        self.config = config or Config()
        self.processors = processors or Processors()
        self.runner = runner

The cleanest way to change this would be to make runner required as below, a potential problem here is if anyone today uses Nornir() without a runner and then adds one later (it's possible this never happens but hard to know):

    def __init__(
        self,
        inventory: Inventory,
        runner: RunnerPlugin,       <--- Change
        config: Optional[Config] = None,
        data: Optional[GlobalState] = None,
        processors: Optional[Processors] = None,
    ) -> None:
        self.data = data if data is not None else GlobalState()
        self.inventory = inventory
        self.config = config or Config()
        self.processors = processors or Processors()
        self.runner = runner

An alternative would be:

    def __init__(
        self,
        inventory: Inventory,
        config: Optional[Config] = None,
        data: Optional[GlobalState] = None,
        processors: Optional[Processors] = None,
        runner: Optional[RunnerPlugin] = None,
    ) -> None:
        self.data = data if data is not None else GlobalState()
        self.inventory = inventory
        self.config = config or Config()
        self.processors = processors or Processors()
        self.runner = runner or load_runner(self.config)       <--- Change
dbarrosop commented 3 months ago

got it, I am starting to think the proposal on this PR is probably the best option. I don't like it is a runtime issue but we'd have to change the signature for this to be a "clean" fix as that's probably not ideal...