Delete filter_params and convert_config from util.py

RESOLVED FIXED

Status

Cloud Services
Server: Core
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: telliott, Assigned: RaFromBRC)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa+])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Assignee: nobody → rmiller
(Reporter)

Updated

7 years ago
Blocks: 676423
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

7 years ago
Created attachment 555003 [details] [diff] [review]
introduce Configifier class and move conversion code

This only resolves half of this ticket.  `filter_params` fix coming soon.
Attachment #555003 - Flags: review?(telliott)
Attachment #555003 - Flags: review?(tarek)
(Reporter)

Comment 2

7 years ago
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)
Attachment #555003 - Flags: review?(telliott) → review+
(Assignee)

Comment 3

7 years ago
(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.
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 ?
(Assignee)

Comment 5

7 years ago
(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.
(Assignee)

Comment 6

7 years ago
Created attachment 555203 [details] [diff] [review]
Doc changes describing proposed new config behavior
Attachment #555203 - Flags: review?(telliott)
Attachment #555203 - Flags: review?(tarek)
(Reporter)

Comment 7

7 years ago
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?
(Reporter)

Updated

7 years ago
Attachment #555203 - Flags: review?(telliott) → review+
(Assignee)

Comment 8

7 years ago
(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.
(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

Updated

7 years ago
Attachment #555203 - Flags: review?(tarek) → review+
(Reporter)

Comment 10

7 years ago
(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
(Assignee)

Comment 11

7 years ago
(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.
(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 ?
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 ?
(Assignee)

Comment 15

7 years ago
(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`).
(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)
(Assignee)

Comment 17

7 years ago
(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.
(Assignee)

Comment 18

7 years ago
Created attachment 555527 [details] [diff] [review]
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.
Attachment #555003 - Attachment is obsolete: true
Attachment #555003 - Flags: review?(tarek)
Attachment #555527 - Flags: review?(telliott)
(Reporter)

Comment 19

7 years ago
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.
Attachment #555527 - Flags: review?(telliott) → review+
(Reporter)

Comment 20

7 years ago
in http://hg.mozilla.org/services/server-core/rev/9eede2585fba
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Comment 21

7 years ago
whoops, ignore that last one
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 22

7 years ago
in https://hg.mozilla.org/services/server-core/rev/10a405f6cb60
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
QA will need steps to reproduce...
Whiteboard: [qa+]
You need to log in before you can comment on or make changes to this bug.