domainaware / parsedmarc

A Python package and CLI for parsing aggregate and forensic DMARC reports
https://domainaware.github.io/parsedmarc/
Apache License 2.0
993 stars 213 forks source link

Centralize the configuration handling #503

Open Szasza opened 6 months ago

Szasza commented 6 months ago

As parsedmarc's functionality gets richer, the functions' parameter lists are getting longer due to the config being parsed on top level and all the values are individually passed down in the call chain. This will yield enormous parameter lists over time.

It would be great to extract the configuration into a standalone class, so it could be included in any depth and accessed in a more lightweight manner, reducing the amount of passed parameters.

seanthegeek commented 6 months ago

I got excited when I saw this subject thinking this was a PR. 😝

This is absolutely needed. It's a mess already! I'm just not sure what is the right way to redesign it. So many people have contributed code over the years that most of the code isn't mine anymore. It's a pain for me to get around the code, let alone someone brand new to the project.

seanthegeek commented 6 months ago

I just reread your post. That design could work!

Szasza commented 6 months ago

@seanthegeek I will work on the implementation then.

seanthegeek commented 6 months ago

Thinking about it more, having all options as a config object would break things for people who are using specific functions of parsedmarc as a library. How about a config dictionary instead that could be passed to existing functions as **kwargs?

Szasza commented 6 months ago

@seanthegeek it can be done, however it will make the functions slightly more complex. I think it may be a better idea to think about which parts/functions of parsedmarc are used as standalone.

Is parsedmarc meant to be a self-contained app or a utility library?

seanthegeek commented 6 months ago

Ideally it should be both, with some core functions used for parsing reports usable as a library, with some parts on top of that like mailbox monitoring being standalone. Separating those into _ prefixed modules is going to take some time. I'll give it more thought when I have some time this weekend.

rodpayne commented 5 months ago

With the configuration in a standalone class whose instance is passed to all functions, how about including the global dictionaries and caches (such as IP_ADDRESS_CACHE, REVERSE_DNS_MAP, etc.) so that every function can access them with only the one reference parameter being passed down the chain? For example, if every function had an argument of parsedmarc_globals, the function could use parsedmarc_globals.opts.dns_timeout, parsedmarc_globals.ip_address_cache, etc., as needed)

Szasza commented 4 months ago

@seanthegeek I think @rodpayne's proposal is close to what you would like to see.

lingfish commented 3 months ago

Would something like dynaconf help here?

Szasza commented 3 months ago

@lingfish I would think so. @seanthegeek thoughts on https://www.dynaconf.com/?

seanthegeek commented 4 weeks ago

Just new getting around to looking at this. Dynaconf looks really nice!

Szasza commented 6 days ago

@seanthegeek if you are happy with the dynaconf approach, I can give the implementation a shot in the next 2 weeks.