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.38k stars 234 forks source link

SimpleInventory crashes with empty hosts file #826

Open mpaccione opened 1 year ago

mpaccione commented 1 year ago

Hi, your code doesn't allow for an empty hosts file. I find this perplexing given that groups and default yaml files have this check?

 def load(self) -> Inventory:
        yml = ruamel.yaml.YAML(typ="safe")

        if self.defaults_file.exists():
            with open(self.defaults_file, "r", encoding=self.encoding) as f:
                defaults_dict = yml.load(f) or {}
            defaults = _get_defaults(defaults_dict)
        else:
            defaults = Defaults()

        hosts = Hosts()
        with open(self.host_file, "r", encoding=self.encoding) as f:
            hosts_dict = yml.load(f) // CRASHES : WHY IS THERE NO or {} HERE??? //

        for n, h in hosts_dict.items():
            hosts[n] = _get_inventory_element(Host, h, n, defaults)

        groups = Groups()
        if self.group_file.exists():
            with open(self.group_file, "r", encoding=self.encoding) as f:
                groups_dict = yml.load(f) or {}

            for n, g in groups_dict.items():
                groups[n] = _get_inventory_element(Group, g, n, defaults)

            for g in groups.values():
                g.groups = ParentGroups([groups[g] for g in g.groups])
ubaumann commented 1 year ago

What is your use case where you need the SimpleInventory but empty?

mpaccione commented 1 year ago

Hi @ubaumann

Initial setup and install of a product to have an empty yaml file for users. Meaning that the main config.yaml references the file paths correctly and things are in the correct locations but it is empty for users to edit. A better process than having them manually create the files themselves and potentially become confused about other configuration and file path details.

What do you think? Is there a better way here given the exact intention?

ubaumann commented 1 year ago

I would add a demo host to the configuration file. Mybe the localhost. Otherwise, you have to wait for the response of the maintainer on how he feels about a PR or implementing your inventory plugin.

I assume it also fails if your host file looks like this?

---
...
mpaccione commented 1 year ago

Correct, it also fails.

Currently I am making a util function that gets the file path from the config yaml and then does a kinda of validation check on the hosts file.

I know the docs say hosts are mandatory so this might be a far fetched request but imo a crash is a really rough way to handle this.

dbarrosop commented 1 year ago

My concern with not crashing and allowing an empty hosts file is that if a user makes a typo and points to a file that doesn't exist (i.e. pointing to hosts.yaml while they actually called it hosts.yml) the whole thing will fail silently. You could argue the same is true for groups and defaults but the main difference is that the need for an empty hosts file is an edge case (first time someone asks for this) while lots of people doesn't have groups and defaults.

What I'd suggest is the following; create your own Inventory plugin that inherits from SimpleInventory, before calling super do your check and return an empty inventory if the file doesn't exist or if it's empty. Otherwise call super and just let SimpleInventory do its job.