Juan Hernandez has posted comments on this change.
Change subject: core: Add local configuration helper
......................................................................
Patch Set 6:
I agree that the duplication of the defaults is not nice. I will look at a way
to improve this.
The fact that /etc/sysconfig/xxx look like shell scripts doesn't mean that they
are, or to be precise, doesn't mean that you can treat them as such.
Introducing things like ${XXX} or $(XXX) is calling for problems. An example is
the files in /etc/sysconfig/network-scripts/ifcfg-*, they are not shell
scripts, and if you treat them as such you get in deep trouble. So in my
opinion it is good practice to avoid shellisms in those files.
In addition the moment you start to use things like "export WHATEVER" or
"WHATEVER=$(XXX)" in that file the job of the engine-setup and engine-upgrade
tools will be extremely complicated. How do you edit the file if you allow
shellisms there? Automatically editing a shell script is certainly difficult.
On the other hand editing a plain configuration/properties file is plain
vanilla.
I think that we already discussed that "/etc/sysconfig" is not probably the
right place for this file, as it is not only service configuration, and other
distributions don't use this directory for this kind of thing. I am open to
relocate it to /etc/ovirt-engine, probably to /etc/ovirt-engine/engine.conf (in
other change, of course) to make it clear that this is not a shell script.
Also take into account that is not only the service that needs to get the
values of the variables in this file. The tools (engine-config and
engine-manage-domains) and the notifier also need to read them, or launched by
different shell scripts. What they have in common (or what they can have if we
ever merge this) is the use of this class to centralize handling of the
parameters, default values, etc.
Another important thing about environment variables and system properties is
that it is impossible to change the values without restarting the program. This
makes them bad for configuration parameters of long running programs, in my
opinion. It is true that we don't have the need to reload any of the parameters
in this file at the moment, but this can change.
--
To view, visit http://gerrit.ovirt.org/6673
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib0106a71c3a1c7c145d4ca836225ce2a26aa115f
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Anonymous Coward #1000055
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Ofer Schreiber <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
