On Thu, Dec 13, 2012 at 2:06 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> 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@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. 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 _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
