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

[Christophe Fergeau]

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

> 
> >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.


> 
> >> 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...

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.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libosinfo/attachments/20121218/442611a5/attachment.sig>