Skip to content

Second iteration - new config files handling #42

Closed
wants to merge 11 commits into from

2 participants

@praiskup

Hi, here is second iteration. I was too lazy to split these big patches into smaller ones, sorry for that. I catched errors previously proposed: showing password when --debug=3 is now away, the connection option has now the same behavior as previously. Default option disappeared (for now). And left the bugzrc.example with small changes. Feel free to change whatever you want.

Pavel

praiskup added some commits Jan 22, 2013
@praiskup praiskup * man/bugz.1: Update date in copyright statement, fix typos. 7ecda51
@praiskup praiskup config: New style of configuration files
Hierarchical configuration files for PyBugz.  Commits for support this
functionality follows this one.

TODO: The 'default' functionality is out.  It will be solved by inheriting.

* bugzrc.example: This is now example of user's config file.
* conf/conf.d/gentoo.conf: Gentoo configuration file.
* conf/conf.d/redhat.conf: Config for Red Hat's bugzilla.
* conf/pybugz.conf: System configuration.
* man/bugz.1: Mention where to look for configuration documentation.
* setup.py: Not sure if this is OK, but try to install configurations.
c83f186
@praiskup praiskup * .gitignore: Ignore 'tags' file and 'build' directory. e40e44e
@praiskup praiskup * bugz/cli.py: Remove unnecessary shebang. 8ea9d18
@praiskup praiskup * bugz/configfile.py: Ok, this is nasty commit. Completely rework
  configuration handlers.  Now the hierarchical approach works.
33a02f0
@praiskup praiskup * bugz/cli.py: Catch exceptions when getting of bug failed. 93a23f2
@praiskup praiskup config & argparser & cli incorporate
* bin/bugz: Move argument processing into PrettyBugz
* bugz/argparsers.py: Setup needed defaults.
* bugz/cli.py: Incorporate new config parsing
66ef36c
@williamh
Owner

I'm curious why the argument processing and config file parsing were moved to the PrettyBugz and not left in bin/bugz? If possible, I think it would be better to handle those outside the PrettyBugz class; However, I can be convinced that putting them in there is the better way.

@praiskup

I just wanted to handle configuration/arguments on one place only. I know that the place is not ideal, this should be probably outside of init method in PrettyBugz, better to extract it into main method on the bin/bugz site.

The reason was probably I wanted to have "easy" interface to allow user to script with PrettyBugz. You can now without dealing by hand with configuration files use PrettyBugz just passing arguments to constructors (and it will handle configurations for you). But I see now, this is not ideal and configurations should be pulled out from PrettyBugz.

@williamh
Owner

Yes, I agree with you, the best place to process the argunents and configuration files is in main(), so can you please update your proposal_2 branch to do this?

Thanks much. :-)

@praiskup

Yup, I'll look at it ;-)

@williamh
Owner

Hey, any luck on this?

@praiskup

William, sorry for the delay and thanks for this ping! I have started to work on this but I got busy. I'll try to finish this asap (hopefully today).

@williamh
Owner

Ok, cool.

Remember the only thing we need is new commits on your proposal_2 branch that move the command line and config file processing back to bin/bugz.

Thanks,

William

@praiskup praiskup Move config/option handling from PrettyBugz to main()
* bin/bugz: Move here.
* bugz/cli.py: Move from here (left only cookie handling).
* bugz/configfile.py: s/password_cmd/passwordcmd/, s/query_statuses/status/,
  remove garbage
5066f30
@praiskup

William, don't you have some idea how to prepare regression testing? It is really difficult to test if everything works OK after such non-trivial changes.

@williamh
Owner

Not exactly, but here is my thought. If we are thinking along the same line, PrettyBugz shouldn't change from what is currently on master, so I think a fair test would be to make sure the args object is built properly after your changes.

@praiskup
@praiskup

Sounds great, but I was thinking about general 'testsuite'. It would
help even in future. I was thinking about script starting apache with
bugzilla on some port, filling database and performing
changes/queries... in testsuite.

Just addition - I'm just thinking about some general solution (maybe
similar to mentioned script). I don't want you to implement my idea.
Actually, I don't know how to write something like this relatively easily.
There are some solutions (e.g. mock & rpm in Fedora) but it is not very
general. But this is probably not good to be discussed here.. It is about
new issue.

Pavel

@williamh
Owner

Yes, a test suite should not be discussed on this pull request; that should be a separate issue.

I'm confused about why we need the type= option. Since it is possible to repeat section names and have the values in later sections override earlier values, I would rather keep the default section instead unless you are planning on adding more section types than "connection" and "settings".

@praiskup

Yes, a test suite should not be discussed on this pull request; that
should be a separate issue.

I'm confused about why we need the type= option. Since it is possible to
repeat section names and have the values in later sections override
earlier values, I would rather keep the default section instead unless you
are planning on adding more section types than "connection" and
"settings".

I'm quite confused too now :). This wasn't really necessary but I just
thought that it would be nice to allow us to add new types of sections in
future. E.g. the separate pull request for [Mimetypes]. There may be
others -> e.g. color mapping (I was planning to give the colorized output
a try). Putting all of this into settings could be hard. Actually,
I thought that we need the default type before (but it may be solved by
default option of settings).

I also don't like the special names for other types of sections as they
may conflict with user's needs (well this may not be real :)).

William, I'll be glad to revert this if you wish.

Pavel

@praiskup

I'm just not sure now that it was good idea. This really needs to be discussed. Just say your opinion.

Pavel

@williamh
Owner

I like everything that has been done in this change except for the "type=" option. Please add a commit to your branch that removes the support for the "type=" option and adds back the default section.

Thanks,

William

@praiskup
praiskup added some commits Feb 2, 2013
@praiskup praiskup config: Restore old semantics for [settings] and [default]
* bin/bugz: Check for config-inherited values.
* bugz/configfile.py: Force all sections inherit from [default], do not look
* for "type" option.
* conf/pybugz.conf: Reflect changes.
4719a8e
@praiskup praiskup config: Allow the option 'inherit' in configuration
* bin/bugz: Bugfix.
* bugz/configfile.py: Calculate the inheritance hierarchy.
* conf/pybugz.conf: Document 'inherit' option.
d965334
@praiskup
praiskup commented Feb 4, 2013

Hi William, have you had some time to look on actual proposal_2 branch?

Thanks, Pavel

@williamh
Owner
williamh commented Feb 4, 2013

Hi Pavel,

I just looked and I like most of what I see, but there are a couple of things I have questions about.

1) What is the relationship between the [settings] and [default] sections? If there is a reason for having these two sections I am interested, but if not, I think we should merge the settings from [settings] into [default].

2) I'm concerned about the inherit option. If all sections have the ability to override the settings in the [default] section, why do we need to inherit from some section other than [default]?

Thanks,

William

@praiskup
praiskup commented Feb 4, 2013

williamh commented an hour ago

Hi Pavel,

I just looked and I like most of what I see, but there are a couple of things
I have questions about.

1) What is the relationship between the [settings] and [default] sections? If
there is a reason for having these two sections I am interested, but if
not, I think we should merge the settings from [settings] into [default].

The [settings] section is global settings related. The [default] connection has
the connection semantics and is related to default connection settings.

I feel bad to specify the 'homeconf' in [default] section, same as I feel bad
with settings the connection related things in [settings]. Other thing is that
the semantic of [default] and [settings] is now completely different. When we
move [default] options into [settings], the semantic will be mixed.. and it will
cause that [settings] will be superset of [connection] which may be confusing
(especially in future).

2) I'm concerned about the inherit option. If all sections have the ability to
override the settings in the [default] section, why do we need to inherit
from some section other than [default]?

This is because user (e.g.) wants to specify its credentials and other settings
in one - let's say [RedHat] connection - but he does not want to copy&paste
everything into very similar connection, e.g. [RedHatScripts]. It is more easy
to inherit from [RedHat] and set quite = True (e.g.). From this point, just the
--connection option is switching between this.

What do you think about it?

Pavel

@williamh
Owner
@praiskup
praiskup commented Feb 4, 2013

If we rename [settings] to [default], we could require a "connection:
connectionname" parameter in the [default] section that points to a
connection, or a --connection connectionname argument on the command
line.
...
Actually it wouldn't if you require a connection to be designated like I
suggested above in the default section or on the command line.

No problem there. I just tried to make the settings more extendable for
future option enhancements. I was trying to separate "connections" like
problems from general "settings". This is nice even from code point of
view. Of course, even the 'homeconf' option (if you like this) may be
specified in default.

I agree that everything may stay as is. Feel free to hack (or reuse or
trash) whatever you want.

INHERITANCE

Ok, this makes sense, since it has to do with connections, but it
introduces a lot of complexity, e.g. how deep can inheritance go?

Should this be limited?

what about inheritance loops?

Needs to be solved.

what about multiple inheritance?

Not needed. Maybe there would be some options to be combined? But ..

What about order?

There should be one fixed-point when one parent is allowed.

I understand what you are trying to do, but is it worth the complexity
or should we just tell users to copy and paste?

Depends on you, really. This sound quite like fun and not so complex. If
you like the inheritance idea, I'm able to fix mentioned problems.

Cheers,
Pavel

@williamh
Owner
williamh commented Feb 5, 2013

We can discuss inheritance on another issue or pull request; right now I am interested in getting the new configuration processing into the tree; I really like a lot of what is going on with this.

Here is another way to look at my concerns about [settings], [default], and a connection.
Looking at a configuration file, I would see two sections -- [settings] and [default] -- Can I override the [settings] section with the [default] section or visa versa?

Also, what happens with the [default] section if I specify a connection in the [settings] section?

Symantically, in my opinion, we should come up with better names, or merge these and call the new section [default]. Another advantage to merging them is that we would preserve backward compatibility.

What do you think?

William

@praiskup
praiskup commented Feb 6, 2013

We can discuss inheritance on another issue or pull request; right now I
am interested in getting the new configuration processing into the tree;
I really like a lot of what is going on with this.

Agreed when I'm not going to be successful in making you agree with
my opinions :).

Here is another way to look at my concerns about [settings], [default],
and a connection. Looking at a configuration file, I would see two
sections -- [settings] and [default] -- Can I override the [settings]
section with the [default] section or visa versa?

No, no at my proposal. At least if I understand you correctly? It was
intentional that it has been divided into two sections. What do you mean
by "override [settings] by with [default]" ?

Also, what happens with the [default] section if I specify a connection
in the [settings] section?

Nothing (again if I'm looking at my proposal) - it is unchanged. The
[default] section is the down-most connection in inheritance hierarchy.
You are able to use 'connection' option in your [settings] section, but it
has "just" the '--connection' effect - you are specifying connection you
want to use. If yours choosed connection does not have specified
something, it will be inherited from its parent (and [default]
respectively).

Symantically, in my opinion, we should come up with better names, or
merge these and call the new section [default]. Another advantage to
merging them is that we would preserve backward compatibility.

What do you think?

The backward compatibility may be fixed by accepting (temporarily)
'connection' also in [default] section. Followed with very verbose
warning.

Pavel

@williamh
Owner
williamh commented Feb 7, 2013

Hang in there with me. ;-) I'm not saying no about inheritance; I'm just saying that I think we are trying to do too much in one pull request, and I would like to revisit inheritance later. :-)

In reading through the code and the documentation for the configuration file parser, I noticed something else I would like to ask you about. You are parsing each config file individually. Is there a reason you didn't pass a list of file paths to cp.read() and have it parse all of the files in one shot? It seems like that would simplify the code and we wouldn't have to worry about the context, stack etc.

I spoke to another user of pybugz today, and he seemed to have the same difficulty I have with understanding the separation between the [default] and [settings] sections. I see them all as default settings, and if you merge [settings] into [default], you can specify values for both the program and default connection settings in one section. The advantage in code for doing all of this is that if you use the [default] section, cp.defaults() gives you a dictionary of all of the default values.

What do you think?

@praiskup
praiskup commented Feb 8, 2013

Hi Will, thanks for comments :)

Hang in there with me. ;-) I'm not saying no about inheritance; I'm just
saying that I think we are trying to do too much in one pull request,
and I would like to revisit inheritance later. :-)

Yes, I know it, but handling of [default] section is somehow related to
inheritance. Separate pull request may follow when needed, though.

In reading through the code and the documentation for the configuration
file parser, I noticed something else I would like to ask you about. You
are parsing each config file individually. Is there a reason you didn't
pass a list of file paths to cp.read() and have it parse all of the
files in one shot? It seems like that would simplify the code and we
wouldn't have to worry about the context, stack etc.

The reason is that at the time we are starting to parse our "first" file
we have no idea about others. If we wanted to have complete list of all
configuration files, we would have to do two runs through all configs -
first gathering names, second doing the work.

I spoke to another user of pybugz today, and he seemed to have the same
difficulty I have with understanding the separation between the
[default] and [settings] sections. I see them all as default settings,
and if you merge [settings] into [default], you can specify values for
both the program and default connection settings in one section.

I have no problem here. Feel free to pick anything useful for pybugz.
I'm just saying that the 'homeconf', 'confdir' (and other settings designed
probably (or hopefully) in future) are not related to a concrete
connection, they are logically unrelated to connection.

The advantage in code for doing all of this is that if you use the
[default] section, cp.defaults() gives you a dictionary of all of the
default values.

Yes, this is true - if the behaviour is as you are saying (haven't checked
it now), it is IMO even more important to separate connection data from
general settings. But just my 2 cents.

Pavel

@praiskup

Hi William, do you still considering this approach?
Pavel

@williamh
Owner
@williamh
Owner

Hello again Pavel,

sorry I've been distracted for so long from this. I am definitely still interested and looking at revisiting it very soon.

:-)

William

@williamh
Owner

Hi Pavel,

after our chat on IRC, I worked more on this, and what I came up with is now in the master branch, and I would like your thoughts on it. commit baa105e does the following:

  1. It creates a connection object which will hold everything from the command line and the config files.
  2. It rewrites the config file module to take advantage of how configparser works and handle multiple config files. It first looks in sys.prefix+/share/pybugz.d/.conf, then /etc/pybugz.d/.conf then it looks for ~/.bugzrc.
  3. It introduces debug logging by dumping the values in the connection object to stdout once it is set up.

Of course, anyone else who wants to do so is welcome to add comments as well.

@williamh
Owner

Hello,

I believe the master branch now covers this. If things are missing still, please feel free to open more pull requests.

Thanks much,

William

@williamh williamh closed this Nov 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.