On Tue, Jun 30, 2009 at 6:54 PM, Mike McCune<[email protected]> wrote: > On 06/30/2009 03:19 PM, Jesus M. Rodriguez wrote: >> >> This most recent commit http://tinyurl.com/mws95e seems to make >> Config.java more and more a KickstartConfig class. >> I know it shouldn't bother me but it does, that Config used to be >> pretty generic a class simply to access the configuration. >> But of late it has become a dumping ground for config strings and >> methods for getting specific entries. In reality, >> I would think a Config class shouldn't change very very rarely to >> support changes maybe in config file format etc. >> >> I'd like to propose a few changes. >> >> 1) move all of the constant definitions to a ConfigConstants class. >> Though I still am *NOT* a fan of defining the >> config entries such as kickstart.virt_bridge, and web.product_name. >> But I do, sort of, understand how folks want to >> see them in Eclipse (though it makes us grep users have to do 2 searches) >> :( >> >> 2) remove the specific methods such as getDefaultVirtCpus() and just >> use the actual Config call >> Config.get().getInt(ConfigConstants.VIRT_CPU, 1) or if folks REALLY >> need these functions, would it be better >> to create a KickstartConfig class that extends Config? I don't really >> like this idea since it might start a cascade >> of a million little config classes. >> > > see below ... > >> 3) I also noticed a bunch of kickstart.* config entries. Where are >> these defined? Do we have a rhn_kickstart.conf >> file? because kickstart.* is an invalid namespace unless we have an >> rhn_kickstart.conf otherwise, what the Config >> class is trying to find a kickstart.* then looking for a web.kickstart.*. > > hm, i added to /etc/rhn/rhn.conf > > kickstart.virt_cpus = 999 > > and the Config picked it up fine. These configs are defined in the code > only. We have lots of instances where there are configurable options in the > code that we don't explicitly define in rhn_web.conf. I've never been a fan > of defining the default in the code and also having to maintain a copy-paste > of that config value in the rhn_web.conf file as well for things that users > don't often change. By placing the values in the code only it allows > Spacewalk users to override the defaults but we don't need to maintain 2 > copies of the config and coders know right there in the code what the > default is. > The config system will find it in rhn.conf just fine. It was made to do that. It reads kickstart.* fine from rhn.conf but it assumes it originally came from rhn_kickstart.conf.
> perhaps I've been wrong all these years in just defining config points in > the code only in the Java code :) I'm ok with code only configuration points. As this allows for extending code in configurable ways. > >> >> Here is the current Config.java: http://pastie.org/530023 >> >> Here is the diff from the current Config to the original one in Spacewalk: >> http://pastie.org/530050 Nothing added was really to do with parsing >> or dealing with the configuration >> system. >> >> Thoughts? Arguments? Rants? :) > > How about option 3: > > ConfigDefaults class that moves all the constants and helper methods to a > class of its own. > > Keep Config.java to be 'pure' and be the mechianics of fetching/storing > configs and then move the 'helper' methods and constants out into its own > file. Sounds better to me. I can live with that :) jesus _______________________________________________ Spacewalk-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/spacewalk-devel
