[virt-tools-list] [libosinfo PATCHv3 10/12] Add osinfo_install_config_new_for_script

[Zeeshan Ali (Khattak)]

On Thu, Dec 13, 2012 at 2:06 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> On Thu, Dec 13, 2012 at 02:56:09AM +0200, Zeeshan Ali (Khattak) wrote:
>> On Wed, Dec 12, 2012 at 5:18 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
>> > In order to be able to automatically transform configuration parameters set
>> > on OsinfoInstallConfig instances (ie translate from generic value to
>> > OS-specific value),
>>
>> Hmm.. this seemingly small change is rather a big change in terms of
>> API design as so far we have kept install config separate from install
>> script. They only meet each other when generating scripts from
>> template.
>>
>> Since the OS-specific values are actually needed when transforming the
>> template to script (i-e in osinfo_install_script_generate_config_xml),
>>  I wonder if there is another less intrusive (in terms of API) way to
>> achieve the goal.
>
> I was initially considering plugging into
> osinfo_install_script_generate_config_xml or some such function, but then
> it made more sense to me to do things the way it's done in this patch
> series.
> The install script XML contains a <config> section describing the valid
> parameters for the install script. Codewise, we have a OsinfoInstallScript
> which can contain a list of OsinfoInstallConfigParam. These objects are the
> result of parsing the install script XML.
>
> We also have an OsinfoInstallConfig class, which, as you mention, is
> independent of the OsinfoInstallScript and OsinfoInstallConfigParam classes
> Note the naming which could be confusing, OsinfoInstallConfig and
> OsinfoInstallConfigParam have nothing to do with each other at the moment.
>
> This separation between OsinfoInstallConfigParam and OsinfoInstallConfig
> means that one has to refer to OsinfoInstallScript to know which parameters
> are valid/required when filling an OsinfoInstallConfig object with data.
>
> Regardless of the datamap stuff, it makes sense to me to associate the
> OsinfoInstallConfig 'schema' (that is a list of
> OsinfoInstallConfigParam) with the OsinfoInstallConfig instance that is
> being filled. This could also allow us in the future to reject setting
> 'useless' arguments, to do argument validation (this arg only has X, Y
> and Z as valid values, this int must be between 1 and 10, ...).
>
> I can look at not associating OsinfoInstallConfigParam with
> OsinfoInstallConfig, but this will require a bit of refactoring,
> and I think this alternative approach would look more hackish than what
> this series is doing.
>
> In short, I consider the subseries:
>
> Add osinfo_install_config_new_for_script
> Add OsinfoInstallConfig::valid-params property
> OsinfoInstallScript: Use an OsinfoInstallConfigParamList
> Set id in osinfo_install_config_param_new
> Add OsinfoInstallConfigParamList
>
> as a nice improvement to the codebase even if there were not the datamap
> changes on top of it.

Thanks for the explanation and I would agree with you to everything
you said here but this kinda breaks API/ABI cause user is now expected
to use a different _new(). I'll repeat the question I asked before:
How does the app know which _new() to use? Now that we'll be expecting
OS-specific values in scripts, any existing code (or even future code
that uses the existing _new()) will still pass us OS-independent
config that did not see the transformation.

Regarding needing a lot of changes for keeping config separate, that
is unfortunately true but keeping all above in mind I'm afraid we
might not have a choice. We can still reuse some of the patches/code
you already wrote though. At least these:

> Add osinfo_install_config_new_for_script
> OsinfoInstallScript: Use an OsinfoInstallConfigParamList
> Set id in osinfo_install_config_param_new
> Add OsinfoInstallConfigParamList

osinfo_install_config_new_for_script() could be an internal function
we can use in osinfo_install_script_generate_config_xml(). I don't
think this very hackish.

Still, If you could suggest how to solve the issue(s) I pointed out in
your approach, I'm more than willing to accept your patches.

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124