[katello-devel] Foreman integration model question

Wed Aug 29 17:23:17 UTC 2012

On 29.08.12 16:36, Dmitri Dolguikh wrote:
> On 29/08/12 12:21 PM, Petr Chalupa wrote:
>>
>>
>> On 23.08.12 17:27, Dmitri Dolguikh wrote:
>>> On 22/08/12 06:07 PM, Justin Sherrill wrote:
>>>> On 08/22/2012 08:26 AM, Petr Chalupa wrote:
>>>>> Lukas is right, our motivation was to break the code into smaller
>>>>> pieces (I am big fan of SOLID and 'single responsibility principle'
>>>>> in particular). If you write code this way, you can then move things
>>>>> around very quickly so you can develop features and fix bugs faster.
>>>>> No to mention it can be testes more easily.
>>>>>
>>>>> app/models/user.rb (User) is representation of katello user which
>>>>> mixes (if foreman is installed) foreman orchestration
>>>>> app/models/glue/forema/user.rb. Orchestration is working with foreman
>>>>> through children of ForemanModel which are representation of remote
>>>>> resources app/models/foreman/user.rb (Foreman::User). Foreman::User
>>>>> is using foreman-api bindings which are defined as singletons in
>>>>> lib/resources/foreman.rb.
>>>>>
>>>>> So there are 4 layers:
>>>>> - katello model
>>>>> - foreman-katello orchestration
>>>>> - foreman model (remote resource)
>>>>> - foreman bindings (gem foreman-api)
>>>>>
>>>>> I also for splitting up big parts.
>>>> I do like this auto generated api method better (aside from there
>>>> being two distinct User models).  To say a downside of the current
>>>> model is a giant single file pulp.rb is kinda moot, as that was just
>>>> how it was organized, we can organize it differently quite easily.
>>>> What we are doing here with foreman is wayyyy more than just
>>>> organizing it differently, its a fairly completely different way of
>>>> orchestrating things, bypassing our lazy_accessor magic.
>>> I will insist on using the orchestration layer for all access outside of
>>> local model. Controller code shouldn't be aware of where a given model
>>> attribute is, otherwise you are leaking orchestration outside of model.
>>
>> I am sorry I did not send an email about it before we started working
>> on this. We had a discussion about it and we agreed that this is
>> better approach, but we didn't feel that this is a big change. Sorry
>> about that. I'll try to explain why I think this is better.
>>
>> The change is that we didn't use lazy_accessor but introduced new
>> object representing remote resources. The reason is that katello
>> models don't get any fatter.
>>
>> I agree that orchestration should be hidden from ActiveController.
>> Because Foreman::User doesn't need to expose any of its attributes
>> it's not clear how it would look like in KTEnvironment example.
>>
>> In KTEnvironment case it would look like this:
>>
>> There would be 4 files:
>> - models/kt_environment.rb - class definition of katello model
>> (KTEnvironment)
>> - models/glue/foreman/kt_environment.rb - module defining foreman
>> orchestration (Glue::Pulp::KTEnvironment)
>> - models/foreman/kt_environment.rb - class definition of foreman
>> remote resource (Foreman::KTEnvironment)
>> - lib/resources/foreman.rb - defines singleton resource adapter using
>> foreman_api (Resources::Foreman::KTEnvironment)
>>
>> Lets assume Foreman::KTEnvironment has two attributes :hidden and
>> :exposed where :hidden should not be visible form ActiveController nad
>> :exposed should.
>>
>> Then :hidden is hidden in Foreman::KTEnvironment and
>> Foreman::KTEnvironment instance accesible from KTEnvironment instance
>> should not be accessed directly, Foreman::KTEnvironment is private
>> represantion managed by foreman orchestration
>> (Glue::Pulp::KTEnvironment).
>>
>> To expose :exposed attribute in KTEnvironment you would define
>> decelerators in Glue::Pulp::KTEnvironment:
>>
>> def exposed
>>   foreman.exposed
>> end
>>
>> def exposed=(val)
>>   foreman.exposed = val
>> end
>>
>> # or method .delegate can by used
>> delegate :exposed, :exposed=, :to => :foreman
>>
>> :exposed is then accessible from ActiveController but everything else
>> is hidden.
>
> Heh. I was thinking of something similar to encapsulate initialization
> of remote attributes and possibly having a longer-living cache shared by
> all remote attributes from a given sub-system.

I would not introduce any cache until it's measured that this a real 
bottleneck and needs to be cached. Caches are often introducing 
hard-to-find bugs, I would avoid them as long as possible.

> We could potentially leave the proxy object in place (instead of hiding
> it behind a delegate) - it would serve as a notice of different call
> semantic.
>
> -d
>>
>> We felt that foreman vs candlepin/pulp orchestration is clear enough
>> border not to rise confusion. As I see it the drawback in consistency
>> is much more less important than advantages of this approach:
>> - no huge objects
>> - easier testing
>> - clearer distribution of responsibilities between objects
>>
>> +1 to keep the current approach
>>
>> I hope this is explaining motivation behind this. If you have more
>> questions please ask.
>>
>> Petr
>>
>>> That aside, but why do we need to *generate* code in *ruby*? Does that
>>> mean we have a lot of dumb code with very little logic in it? We can and
>>> should do better.
>>>
>>> -d
>>>>
>>>> For example, lets say we do the same with environment, where there is
>>>> a katello environment and a foreman environment.  And lets say there
>>>> is some special piece of information only stored in foreman (this is
>>>> not a far-fetched scenario).
>>>>
>>>> With our current orchestration you would do something like:
>>>>
>>>> a = KTEnvironment.find(3)
>>>> print a.remote_attribute
>>>> a.remote_attribute = "foo"
>>>> a.save!
>>>>
>>>> whereas the way i see it now, we'd have to do:
>>>>
>>>> a = KTEnvironment.find(3)
>>>> a.foreman_user.remote_attribute = "foo"
>>>> a.save!
>>>>
>>>> I don't really like this better as you'd have to remember which
>>>> backend engine a particular attribute is.  Today you don't.  I don't
>>>> really expect any of these arguments to change the way we are doing
>>>> anything, its just annoying to change our entire way of working with a
>>>> backend engine without any discussion :)
>>>>
>>>> We need to be aware that doing it this way is creating a 2nd way with
>>>> working with fully (or partially) remote objects and that needs to be
>>>> clear and apparent.  It also needs to be justified :)
>>>>
>>>> -Justin
>>>>
>>>>>
>>>>> Petr
>>>>>
>>>>> On 22.08.12 10:19, Lukas Zapletal wrote:
>>>>>> On Tue, Aug 21, 2012 at 02:48:56PM -0400, Justin Sherrill wrote:
>>>>>>> Taking user for example we would have:
>>>>>>>
>>>>>>> user.rb
>>>>>>> glue/pulp/user.rb
>>>>>>
>>>>>> But you are missing one piece of the puzzle:
>>>>>>
>>>>>> lib/resources/pulp.rb (class User)
>>>>>>
>>>>>> And that is the piece which you can find in foreman/user.rb.
>>>>>>
>>>>>>> What I see with foreman is:
>>>>>>>
>>>>>>> user.rb
>>>>>>> glue/foreman/user.rb
>>>>>>> foreman/user.rb
>>>>>>
>>>>>> So instead having all the resources in a single class
>>>>>> (lib/resources/foreman.rb), we are going to have (or better -
>>>>>> generate)
>>>>>> all resource (read also as "dumb") classes. Each in separate file.
>>>>>>
>>>>>>> Where foreman/user is a second class that can be instantiated and
>>>>>>> used on its own, while glue/foreman/user.rb is simply the
>>>>>>> orchestration to create that object.  I see the use of
>>>>>>> Resources::ForemanModel, but in this user instance the foreman user
>>>>>>> could be manipulated completely apart from the katello user (which
>>>>>>> is bad IMHO).  This also is completely different from our existing
>>>>>>> orchestration methods.
>>>>>>
>>>>>> Yes, it is different. I like this approach more. Creating a one
>>>>>> instance
>>>>>> per request is nothing, while the code is definitely better than
>>>>>> eight
>>>>>> hundred lines long pulp.rb with all the resources there as "class"
>>>>>> methods.
>>>>>>
>>>>>> I think this is the correct approach and we may discuss splitting
>>>>>> those
>>>>>> big resource files in future into smaller pieces for other backend
>>>>>> engines too. If we write a similar generator, we wont longer need to
>>>>>> write them manually (and we will be able to track API changes too).
>>>>>>
>>>>>> I was not writing the code, but I believe those instances are treated
>>>>>> like singletons, so you do not need to create instances by yourself:
>>>>>>
>>>>>> https://github.com/Katello/katello/blob/foreman_architectures/src/lib/resources/foreman.rb
>>>>>>
>>>>>>
>>>>>>
>>>>>> Guys please elaborate what the intentions are.
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> katello-devel mailing list
>>>>> katello-devel at redhat.com
>>>>> https://www.redhat.com/mailman/listinfo/katello-devel
>>>>
>>>> _______________________________________________
>>>> katello-devel mailing list
>>>> katello-devel at redhat.com
>>>> https://www.redhat.com/mailman/listinfo/katello-devel
>>>
>>>
>>> _______________________________________________
>>> katello-devel mailing list
>>> katello-devel at redhat.com
>>> https://www.redhat.com/mailman/listinfo/katello-devel
>>
>> _______________________________________________
>> katello-devel mailing list
>> katello-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/katello-devel
>
>
> _______________________________________________
> katello-devel mailing list
> katello-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/katello-devel