Hadron / carthage

Carthage is an Infrastructure as Code (IAC) framework
Other
7 stars 4 forks source link

When supplementary_injection_keys overlaps with a provider registered in a model's __init__ or __new__, the error is confusing #17

Closed kdienes closed 1 year ago

kdienes commented 2 years ago

If I put: xxx = ACESPodmanContainer

in a model, where ACESPodmanContainer inherits from Machine, I get an ExistingProvider exception later when the model tries to provide MachineImplementation.

I made it work with: xxx = dependency_quote(RHELPodmanContainer)

This might be explained in the Carthage documentation but a quick read-through wasn't able to find.

hartmans commented 2 years ago

What do you mean provide MachineImplementation?

kdienes commented 2 years ago

This is the line causing the error, because ACESPodmanContainer has already been registered for InjectorKey(Machine)

https://github.com/Hadron/carthage/blob/b3161a34731b08fb7a883838990521ad8efc4706/carthage/modeling/base.py#L339

hartmans commented 2 years ago

Okay, so we can definitely do better than that error. What do you think the right behavior should be? Override your registration of InjectionKey(Machine)?

kdienes commented 2 years ago

I'm not certain that I understand what's happening, so let me check.

The RHS of the assignment isn't getting used as an actual value, but instead getting resolved as an InjectionKey. So as a result of that resolution it gets added as a provider in the model injector. Adding dependency_quote keeps it from getting registered as a provider.

If that's correct, then I don't think I want it to replace the registration. (Speaking as a naive developer,) I don't want to register RHELPodmanContainer as a provider host at all. I just wanted it to be available to models created later to know what kind of containers to create. So I was surprised it was getting registered as a provider at all.

I'm mostly sure that I get why it is, and I'm not sure that there's an easy cure for my surprise other than explaining more thoroughly what assignment in a model context actually does. But I'll wait until I understand assignment better before having an opinion there.

My "show me where the previous registration happened" patch helped a lot as well.

hartmans commented 1 year ago

Oops, I failed to notice you had replied. When you do foo = rhs in an InjectableModel and rhs is a type, then you end up doing something like

add_provider(InjcetionKey("foo"), rhs)
foo = injector_access(InjcetionKey('foo'))

So, you register as an injector and as a property on the class that will memoize the result of self.injector.get_instance("foo"). When injector.add_provider is called, it looks at k.supplementary_injection_keys on k, the key being registered. That ends up calling supplementary_injection_keys on any Injectable. So your container class's supplementary_injection_keys is getting called and apparently eventually yields InjectionKey(Machine). so you end up getting registered as INjectionKey(Machine).

Then, later, at least MachineModel tries to register its own provider for InjectionKey(Machine), namely something that will instantiate a instance of whatever provides machine_implementation_keyy.

The existing modeling documentation does talk about how assignments work; see the documentation of INjectableModel. It does not go into how that can interact with suplementary_injection_keys.

I definitely think that for your use case you want the dependency_quote. I think the remaining questions are whether we can improve the error and how to handle the documentation.