[katello-devel] Foreman integration model question

Thu Aug 30 10:29:46 UTC 2012

Pull request for foreman integration was created 
https://github.com/Katello/katello/pull/546

On 29.08.12 13:21, 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.
>
> 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