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@xxxxxxxxxx> 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. Christophe
Attachment:
pgp77y2aj8cX7.pgp
Description: PGP signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
