675347 - Delete filter_params and convert_config from util.py

Status: RESOLVED FIXED

(Cloud Services :: Server: Core, defect)

Description

[Toby Elliott [:telliott]]

If there's something in these functions that isn't provided by the Config class, we should figure that out and move them there. If not, time to change everything over and remove them from util.py.

Comment 1

[Rob Miller [:rmiller]]

Attachment: introduce Configifier class and move conversion code

This only resolves half of this ticket.  `filter_params` fix coming soon.

Comment 2

[Toby Elliott [:telliott]]

Comment on attachment 555003 [details] [diff] [review]
introduce Configifier class and move conversion code

Looks good with the following caveats:

1) I dislike the name Configifier ;)
2) I'm assuming that this is an intermediate stage, as I think eventually:

        configifier = Configifier(global_conf)
        params = configifier.config

should be 

       params = Config(global_conf)

Comment 3

[Rob Miller [:rmiller]]

(In reply to Toby Elliott [:telliott] from comment #2)
> Comment on attachment 555003 [details] [diff] [review]
> introduce Configifier class and move conversion code
> 
> Looks good with the following caveats:
> 
> 1) I dislike the name Configifier ;)

You would.  ;)

> 2) I'm assuming that this is an intermediate stage, as I think eventually:
> 
>         configifier = Configifier(global_conf)
>         params = configifier.config
> 
> should be 
> 
>        params = Config(global_conf)

Yes.  W/ the current implementation (where the "Configifier" is NOT actually the configuration itself, just the worker that prepares it) this doesn't make sense, but if we go w/ a "dict-plus" config object as we were discussing today, your example is what it would look like.

Comment 4

[Tarek Ziadé (:tarek)]

I am not sure to understand the goal of Configifier.

Right now we have a Config-like object and a few functions around that filters dict-like or config-like object. 

I think these functions should migrate as methods or maybe class methods, in the config object like this, and any intermediate class removed.

The use case of dealing with Configifier at any level is just an extra indirection we don't need, unless Configifier is meant to provide extra features ?

Comment 5

[Rob Miller [:rmiller]]

(In reply to Tarek Ziadé (:tarek) from comment #4)
> I am not sure to understand the goal of Configifier.

Configifier is an intermediate step towards better encapsulation of all the config parsing and converting functionality.  The Config class (renamed in my patch) did parsing, but also was an intermediate data structure; it was invoked by the `convert_config` function in the services.util module.  This change puts all of the config code in the same place and provides a class that client code can interact with that hides all the details of how configuration is actually processed.

This is not intended to be the final API, though; I'll be attaching a patch to the documentation shortly that describes the proposed end goal.

> Right now we have a Config-like object and a few functions around that
> filters dict-like or config-like object. 
> 
> I think these functions should migrate as methods or maybe class methods, in
> the config object like this, and any intermediate class removed.

We're in agreement here, but with a small difference.  The ConfigParser base class that we use is useful as a parser, but it's a crappy data structure IMO b/c it sort of looks like a dictionary but doesn't actually behave like one (the `items` method requires a `section` argument, for example).

Toby and I thought we should use our own data structure, which would be a dictionary with the addition of a single `get_section` method that would replace `filter_params`.

> The use case of dealing with Configifier at any level is just an extra
> indirection we don't need, unless Configifier is meant to provide extra
> features ?

The indirection was really just to keep the client code from interacting w/ the ConfigParser class, so we could be more certain that the broken dict-like config object wouldn't leak out and confuse matters.  The next code patch on this ticket will do away with Configifier and will replace it with our own Config object.

Comment 6

[Rob Miller [:rmiller]]

Attachment: Doc changes describing proposed new config behavior

Comment 7

[Toby Elliott [:telliott]]

Comment on attachment 555203 [details] [diff] [review]
Doc changes describing proposed new config behavior

I'm a fan, obviously.

Does .get_section() return global conf, an empty dict, or raise an error?

Comment 8

[Rob Miller [:rmiller]]

(In reply to Toby Elliott [:telliott] from comment #7)
> Comment on attachment 555203 [details] [diff] [review]
> Doc changes describing proposed new config behavior
> 
> I'm a fan, obviously.
> 
> Does .get_section() return global conf, an empty dict, or raise an error?

I don't feel very strongly about it, but I lean towards get_section('') and get_section('global') both returning the global conf, w/ get_section() raising an error.

Comment 9

[Tarek Ziadé (:tarek)]

(In reply to Rob Miller [:RaFromBRC :rmiller] from comment #5)
> (In reply to Tarek Ziadé (:tarek) from comment #4)
> > I am not sure to understand the goal of Configifier.
> 
> Configifier is an intermediate step towards better encapsulation of all the
> config parsing and converting functionality.  The Config class (renamed in
> my patch) did parsing, but also was an intermediate data structure; it was
> invoked by the `convert_config` function in the services.util module.  This
> change puts all of the config code in the same place and provides a class
> that client code can interact with that hides all the details of how
> configuration is actually processed.
> 
> This is not intended to be the final API, though; I'll be attaching a patch
> to the documentation shortly that describes the proposed end goal.

I am not sure to understand why we want an intermediate state. It seems that we are in agreement that we should have a single config class, and change a bit its APIs. Deriving from ConfigParser or not is a detail imo, and if we don't we just need to have a clearly defined set of methods.

Can you list those methods in the doc so we get an overview ?


> > Right now we have a Config-like object and a few functions around that
> > filters dict-like or config-like object. 
> > 
> > I think these functions should migrate as methods or maybe class methods, in
> > the config object like this, and any intermediate class removed.
> 
> We're in agreement here, but with a small difference.  The ConfigParser base
> class that we use is useful as a parser, but it's a crappy data structure
> IMO b/c it sort of looks like a dictionary but doesn't actually behave like
> one (the `items` method requires a `section` argument, for example).
> 
> Toby and I thought we should use our own data structure, which would be a
> dictionary with the addition of a single `get_section` method that would
> replace `filter_params`.

only get_section() ? 

> 
> > The use case of dealing with Configifier at any level is just an extra
> > indirection we don't need, unless Configifier is meant to provide extra
> > features ?
> 
> The indirection was really just to keep the client code from interacting w/
> the ConfigParser class, so we could be more certain that the broken
> dict-like config object wouldn't leak out and confuse matters.  The next
> code patch on this ticket will do away with Configifier and will replace it
> with our own Config object.

Ah ok cool.

So to summarize:

- a new dict-like Config class from scratch with <methods list to be defined>
- the server-specific sections I've added in storage, included as a feature of the Config class

sounds good to me

Comment 10

[Toby Elliott [:telliott]]

(In reply to Rob Miller [:RaFromBRC :rmiller] from comment #8)

> I don't feel very strongly about it, but I lean towards get_section('') and
> get_section('global') both returning the global conf, w/ get_section()
> raising an error.

I don't actually care, as long as it's defined, but I'd want ('') and () to behave the same way. But that may just be me being all Perl-y

Comment 11

[Rob Miller [:rmiller]]

(In reply to Tarek Ziadé (:tarek) from comment #9)
> (In reply to Rob Miller [:RaFromBRC :rmiller] from comment #5)
> > This is not intended to be the final API, though; I'll be attaching a patch
> > to the documentation shortly that describes the proposed end goal.
> 
> I am not sure to understand why we want an intermediate state.

Well, the intermediate state was implemented before the ideas specified in the documentation were finalized.  You can ignore or r- the patch, it'll be superseded by a new one this week.

> It seems that
> we are in agreement that we should have a single config class, and change a
> bit its APIs. Deriving from ConfigParser or not is a detail imo, and if we
> don't we just need to have a clearly defined set of methods.
> 
> Can you list those methods in the doc so we get an overview ?

Will do, but as we imagine it right now it will have all of the methods and behavior of a normal dictionary, with two additions:

- load_config(cfgdict): Accepts a dictionary, runs it through the conversion and loads config from any nested `file:` entries, invoked by the constructor.  Already exists in the Configifier class.

- get_section(section): As described in the docs.

> > > Right now we have a Config-like object and a few functions around that
> > > filters dict-like or config-like object. 
> > > 
> > > I think these functions should migrate as methods or maybe class methods, in
> > > the config object like this, and any intermediate class removed.
> > 
> > We're in agreement here, but with a small difference.  The ConfigParser base
> > class that we use is useful as a parser, but it's a crappy data structure
> > IMO b/c it sort of looks like a dictionary but doesn't actually behave like
> > one (the `items` method requires a `section` argument, for example).
> > 
> > Toby and I thought we should use our own data structure, which would be a
> > dictionary with the addition of a single `get_section` method that would
> > replace `filter_params`.
> 
> only get_section() ?

and load_config().
 
> > > The use case of dealing with Configifier at any level is just an extra
> > > indirection we don't need, unless Configifier is meant to provide extra
> > > features ?
> > 
> > The indirection was really just to keep the client code from interacting w/
> > the ConfigParser class, so we could be more certain that the broken
> > dict-like config object wouldn't leak out and confuse matters.  The next
> > code patch on this ticket will do away with Configifier and will replace it
> > with our own Config object.
> 
> Ah ok cool.
> 
> So to summarize:
> 
> - a new dict-like Config class from scratch with <methods list to be defined>
> - the server-specific sections I've added in storage, included as a feature
> of the Config class
> 
> sounds good to me

Great, I'll get started on it.

Comment 12

[Tarek Ziadé (:tarek)]

(In reply to Rob Miller [:RaFromBRC :rmiller] from comment #11)
...
> - load_config(cfgdict): Accepts a dictionary, runs it through the conversion
> and loads config from any nested `file:` entries, invoked by the
> constructor.  Already exists in the Configifier class.

What happens if I call it twice ? does it override existing values ?

What about file loading ? I would expect the class to give me a way to load a file, so I don't have to do this as a pre-step.

I think that should be the constructor because it's the most common case

e.g.:   config = Config('/some/file.cfg')
  

or do you have other plans for this ?

Comment 13

[Tarek Ziadé (:tarek)]

ah, I just saw this: "any nested `file:` entries."

What's a "file:" entry ? 

Are you changing the specification of the configuration file ? see https://wiki.mozilla.org/index.php?title=Services/Sync/Server/GlobalConfFile

Or is the dict received is something else than a parsed file ?

Comment 14

[Tarek Ziadé (:tarek)]

oh, I moved the spec here: http://docs.services.mozilla.com/server-devguide/confspec.html

Comment 15

[Rob Miller [:rmiller]]

(In reply to Tarek Ziadé (:tarek) from comment #12)
> (In reply to Rob Miller [:RaFromBRC :rmiller] from comment #11)
> ...
> > - load_config(cfgdict): Accepts a dictionary, runs it through the conversion
> > and loads config from any nested `file:` entries, invoked by the
> > constructor.  Already exists in the Configifier class.
> 
> What happens if I call it twice ? does it override existing values ?

Yes.

> What about file loading ? I would expect the class to give me a way to load
> a file, so I don't have to do this as a pre-step.
> 
> I think that should be the constructor because it's the most common case
> 
> e.g.:   config = Config('/some/file.cfg')
>
> or do you have other plans for this ?

Actually, that's not the most common case, b/c as of right now our initial config is being parsed by PasteDeploy and is handed to us as a dictionary, and this is what is being passed in.

That being said, it does make sense to add a `load_from_file` method, and to support a `cfgfile` arg for the constructor (in addition to `cfgdict`).

Comment 16

[Tarek Ziadé (:tarek)]

(In reply to Rob Miller [:RaFromBRC :rmiller] from comment #15)
> (In reply to Tarek Ziadé (:tarek) from comment #12)
> > (In reply to Rob Miller [:RaFromBRC :rmiller] from comment #11)
> > ...
> > > - load_config(cfgdict): Accepts a dictionary, runs it through the conversion
> > > and loads config from any nested `file:` entries, invoked by the
> > > constructor.  Already exists in the Configifier class.
> > 
> > What happens if I call it twice ? does it override existing values ?
> 
> Yes.
> 
> > What about file loading ? I would expect the class to give me a way to load
> > a file, so I don't have to do this as a pre-step.
> > 
> > I think that should be the constructor because it's the most common case
> > 
> > e.g.:   config = Config('/some/file.cfg')
> >
> > or do you have other plans for this ?
> 
> Actually, that's not the most common case, b/c as of right now our initial
> config is being parsed by PasteDeploy and is handed to us as a dictionary,
> and this is what is being passed in.

But I thought we were getting rid of the Paster layer in favor of a direct loading of the config ?

> 
> That being said, it does make sense to add a `load_from_file` method, and to
> support a `cfgfile` arg for the constructor (in addition to `cfgdict`).

I suggest: s/cfgfile/path   (file is already in the method name)

Comment 17

[Rob Miller [:rmiller]]

(In reply to Tarek Ziadé (:tarek) from comment #13)
> ah, I just saw this: "any nested `file:` entries."
> 
> What's a "file:" entry ? 
> 
> Are you changing the specification of the configuration file ? see
> https://wiki.mozilla.org/index.php?title=Services/Sync/Server/GlobalConfFile
> 
> Or is the dict received is something else than a parsed file ?

I'm not changing the spec, I'm just dealing w/ how we currently bootstrap things, which is that we're given a parsed PasteDeploy config w/ a `configuration` entry that points to the app config.  This matches the behavior of the `convert_config` function that was replaced.

I know that we don't want to stay wedded to PasteDeploy, and that we'd want to add our own file loading support, but I hadn't yet wedged it in to the API.  I can add the `load_from_file` method from the outset, though, so it's ready to be used when we need it.

Comment 18

[Rob Miller [:rmiller]]

Attachment: custom Config class encapsulating conversion and section filtering

This implements everything described in the documentation changes EXCEPT for the "multi-host" config settings, for which I will open a new bug.

Comment 19

[Toby Elliott [:telliott]]

Comment on attachment 555527 [details] [diff] [review]
custom Config class encapsulating conversion and section filtering

Looks good. 2 notes:

1) consider reversing the param order on the Config __init__ function. I think the most common semantic is going to be conf = Config(filename)

2) not really related to this patch, but I'm not sure why we have the 'here' value stuck into the config at random. The seems to violate all kinds of abstractions, but I don't understand the use case well enough to be able to speculate cogently on it.

Comment 20

[Toby Elliott [:telliott]]

in http://hg.mozilla.org/services/server-core/rev/9eede2585fba

Comment 21

[Toby Elliott [:telliott]]

whoops, ignore that last one

Comment 22

[Rob Miller [:rmiller]]

in https://hg.mozilla.org/services/server-core/rev/10a405f6cb60

Comment 23

[James Bonacci [:jbonacci]]

QA will need steps to reproduce...