kapicorp / kapitan

Generic templated configuration management for Kubernetes, Terraform and other things
https://kapitan.dev
Apache License 2.0
1.8k stars 198 forks source link

Fix reclass #1135

Closed ademariag closed 5 months ago

ademariag commented 7 months ago

Fixes #

Proposed Changes

Docs and Tests

ademariag commented 7 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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)