[Libosinfo] [PATCHv4 06/11] Add OsinfoInstallConfig:config-params property

[Zeeshan Ali (Khattak)]

On Tue, Dec 18, 2012 at 8:15 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> On Tue, Dec 18, 2012 at 07:57:00PM +0200, Zeeshan Ali (Khattak) wrote:
>> On Tue, Dec 18, 2012 at 7:38 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
>> > On Tue, Dec 18, 2012 at 07:18:04PM +0200, Zeeshan Ali (Khattak) wrote:
>> >> On Tue, Dec 18, 2012 at 6:43 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
>> >> > In my opinion, the second approach is more convenient both for the library
>> >> > user and for us from a maintainance point of view, which explains my
>> >> > insistance on moving this way ;)
>> >>
>> >> Thanks for the summary. Thing is that we have already moved a lot
>> >> towards the first approach
>> >
>> > Are you saying the OsinfoInstallScript/OsinfoInstallConfig/
>> > OsinfoInstallConfigParam relationship was intentionally designed this way?
>> > Ie OsinfoInstallConfigParam and OsinfoInstallConfig are separate on
>> > purpose?
>>
>> Yes.
>
> Well, if that's the case and if that was obvious to you, I'm not very
> pleased that you mention that now after another patch iteration rather than
> in answer to
> https://www.redhat.com/archives/virt-tools-list/2012-December/msg00288.html

I'm sorry, I just got a bit lost in these changes.

>> >Until now my feeling was that this ended up being implemented
>> > this way, but that this was not a conscious decision, especially as this
>> > code didn't land very long ago.
>>
>> AFAIK, the code in question has been in a separate branch/list for a
>> while and was thoroughly reviewed.
>
> This was still a huge chunk of code, it's easy to miss some things during
> review.

I can understand and I didn't mean to blame you. There were some other
problematic changes that I also missed during the reviews.

>> >> and to be able to move towards the second
>> >> approach, we must deprecate the API that are designed for the first
>> >> (existing) approach. Otherwise we'll just be confusing app developers
>> >> with this contradiction.
>> >
>> > I'm not exactly sure what API you have in mind exactly. All I'm suggesting
>> > is gradual improvement and having a clear idea of what belongs where. I
>> > don't think we have things to deprecate to achieve that.
>>
>> I'm talking about having this config-params on both InstallScript and
>> InstallConfig. As you pointed out already, currently app has to look
>> at the params on the InstallScript to see which parameters the script
>> will be able to use and which ones it must be provided.
>
> Actually, OsinfoInstallScript does not expose an
> OsinfoInstallConfigParamList as this class is introduced by this series.
> Not having this class in the first place is one of the reasons I assumed
> this part of the code may need some improvements...

I did indeed forget that fact that the list itself is not being
exposed as is but rather InstallScript only provides methods to access
params in the list.

> We have various methods on the OsinfoInstallScript class to handle
> OsinfoInstallConfigParam, but they look out of place to me, especially
> after the introduction of the OsinfoInstallConfigParamList class. I
> wouldn't mind deprecating these methods, they are probably not widely used
> anyway.

None of the libosinfo API is yet widely used. The main app that uses
libosinfo is Boxes and we are using the methods in question in there.

As I said before, I don't think there is a compelling reason to
deprecate APIs in this case. Hence the reason I wanted Daniel to make
the decision here.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124