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.
perhaps I've been wrong all these years in just defining config points
in the code only in the Java code :)
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.
Mike
_______________________________________________
Spacewalk-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/spacewalk-devel
