Fix reclass

[ademariag]

Fixes #

Proposed Changes

• reads the reclass_config file only once
• initialises the inventory for reclass only once
• passes compose_node_name correctly
• detects mismatch in commpose_node_name

Docs and Tests

• Tests added
• Updated documentation

[MatteoVoges]

I don't have much time right now, but at first glance, it looks like that this adds several arguments which I highly wanted to avoid. Maybe you could add why these changes are needed. compose_node_name may not be supported right now and I have to look into it, but passing it to the get_inventory() method violates the encapsulation of the inventory.

[ademariag]

I don't have much time right now, but at first glance, it looks like that this adds several arguments which I highly wanted to avoid. Maybe you could add why these changes are needed. compose_node_name may not be supported right now and I have to look into it, but passing it to the get_inventory() method violates the encapsulation of the inventory.

I believe I have addressed your concerns and reduced the change to the essential

[MatteoVoges]

Thanks for the refactoring, I still see some issues with these changes:

1. We should think of a cleaner and generic way to have global arguments, instead of setting each argument to global. I would propose a change to the scope of the arguments, so that we don't have a <cmd-name> namespace and a global namespace, but have namespaces (and parsers) per core-kapitan-functionality (e.g. compile, inventory, logging, dumping .... Then we would have all arguments regarded to the inventory in one group and get several benefits out of it.

2. The Inventory-Interface was designed, to have a standardised way to get the inventory ressources. With overwriting the __init__ method for reclass, we break out of this conzept and lose the standardisation. We should render the inventory in render_targets().

3. compose_node_name is now a kapitan-concept and can be applied to all inventory_backends. When reading the configfile for reclass, we simply can override the config dictionary and read the value from the kapitan args. The current solution (before the interface) where you had to configure the reclass_config.yml, was not the best, because kapitan should control the behavior of its inventory and there should no changes be made in inventory-specific configfiles.

Sorry for that essay, but I designed the interface with improvements to encapsulation and clear responsibilties in mind, and I still think, we should keep these. If there are any concerns or clear advantages for these changes, let us dicuss them!

[MatteoVoges]

Hey, I've created #1138 to fix compose_node_name in a slighty different way. Again @ademariag the identity of parameters.kapitan.vars.target is a bit strange. Maybe we discuss about this next week?

[simu]

I didn't see this PR on Friday, but I think #1141 partially addresses the excessive amount of times that Kapitan 0.33.x renders the Reclass inventory. I have no hard preference on whether we go with the more substantial changes here or my minimal fix in #1141.

From the PR description:

initialises the inventory for reclass only once

Note that there's cases where we actually need to re-render the Reclass inventory (e.g. after fetching remote inventories).

[MatteoVoges]

the excessive amount of times that Kapitan 0.33.x renders the Reclass inventory

Can you explain, what do you mean here? Because when I tested, I ensured that it renders only once in the actual inventory rendering step. Afterwards its used from the cache. And fetching dependencies or remote inventories works fine as well.

I tested it with my fix #1138, so the current version may be broken.

[simu]

Can you explain, what do you mean here? Because when I tested, I ensured that it renders only once in the actual inventory rendering step. Afterwards its used from the cache. And fetching dependencies or remote inventories works fine as well.

The currently released version (and as far as I can see #1138 as well) has a mismatch between Inventory.get_targets() which calls self.render_targets(targets=[]) when it sees that all targets are cached, and ReclassInventory.render_targets() which unconditionally always renders the Reclass inventory when it's called (regardless of the contents of parameter targets)