[katello-devel] Foreman integration model question

Wed Aug 29 11:21:02 UTC 2012

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.

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