kapply panic when the inventory object is missing

[gautierdelorme]

Seems like this change https://github.com/kubernetes-sigs/cli-utils/pull/457 makes kapply (and other tools relying on the lib) to panic when apply is ran against a directory without inventory object. Instead I would expect an error message to show up like it used to: https://github.com/kubernetes-sigs/cli-utils/blob/42d34da8936daf29f55d296f2a884e5062f4c49d/pkg/inventory/inventory_error.go#L15-L21

[karlkfi]

The PR you linked was @seans3's attempt to pursue making the inventory optional. But I don't think it went all the way. I don't see any tests added for what happens when there's no inventory. And the PR removed the Missing inventory object is false unit test. It looks like the intended behavior was to return nil if there was no inventory, and then the nil inventory would be passed to inventory.WrapInventoryInfoObj(invObj), which is specific to whatever inventory provider you're configured to use.

What inventory provider are you using?

[gautierdelorme]

@karlkfi configmap? So probably this code https://github.com/kubernetes-sigs/cli-utils/blob/0ba75be41275cf813da3c0837fc59b9a9c177515/pkg/inventory/inventorycm.go#L34-L39 ?

[karlkfi]

Makes sense that it would panic then. There's a lot of unguarded usages of ConfigMap.inv in there.

@seans3 do you have time to either make inventory fully optional or re-add the nice error check?

I think kpt always adds an inventory object, generated from its Kptfile. And ConfigSync also builds its own inventory object from the R*Sync metadata. So it's possible we don't have any real tests for this use case.

[karlkfi]

If not, I would accept a PR from @gautierdelorme that just adds back the error, and we can put Optional Inventory as a feature request back on the backlog.

[gautierdelorme]

Here is the fix if we decide to move forward with it: https://github.com/kubernetes-sigs/cli-utils/pull/597.